-
-
Notifications
You must be signed in to change notification settings - Fork 795
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clean up the generated form for application registration and update views #1503
Conversation
Allowing the user to select their own arbitrary client ID and secret is a feature. Also, allowing the checkbox for whether the user wants hashing is also a feature. I don't see why one would want to remove those. Maybe I'm misunderstanding the description; I have not look at the code changes yet. |
I don't understand why allowing application users to select their own arbitrary client ID and client secret is considered a feature. Care to explain why? Oauth.com has this to say about client IDs:
It's a weak recommendation, but allowing application developers to choose their own client ID values likely increases the likelihood that the client ID is guessable by third parties. Regarding the client secret:
This is a stronger recommendation than the client ID, and again allowing an application developer to set an arbitrary client secret value definitely increases the likelihood that the secret is guessable. To me, that is not ideal. Furthermore...
This is part of what this PR is implementing - only displaying the client secret one time, which is a widely popular behavior and I would consider best practice.
As far as I know, the existing behavior does not do either of these things. If I have missed anything please point me to it. Lastly, regarding the I did this because it's nearly impossible to go from hashed to unhashed, so it makes no sense (to me) to display the checkbox if the client secret has already been hashed. I will concede that it does still make sense to allow application developers who originally registered their app with an unhashed client secret to change that decision by showing the |
…play the client secret once
f836b41
to
571b1a8
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1503 +/- ##
==========================================
- Coverage 97.41% 97.19% -0.22%
==========================================
Files 34 34
Lines 2164 2173 +9
==========================================
+ Hits 2108 2112 +4
- Misses 56 61 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This code looks good but I have concerns: Reasons to allow setting the
The oauth2_provider admin pages are only available to admins, not general django auth users, so they shouldn't be arbitrarily constrained from doing what they need to do in their workflow for registering applications (clients). As I see it this PR would introduce breaking changes for those who might rely on the existing functionality. Is there a way you can add a oauth2_provider setting boolean that enables this functionality only when it is True? A default value for now would be False, with a documented recommendation to set it to True. Since it's changes to the html templates, this might get a bit ugly. @dopry what are your thoughts? |
I agree. We shouldn't show the client secret only once. Other idps, auth0, okta, google, Amazon, make the secret accessible after generation. It may make sense to prevent editing, but I see value in the migration use cases as well |
Actually, unless hashing is unchecked, we do only show the secret once, so @blag 's change to that part is to use a popup to display it one time vs. the current implementation which shows it in the clear on the form until one hits "Save". So there is now way to recover the secret after saving, however the secret can be reset (and rehashed) via modification. |
I can understand this use case.
I'm assuming here that "obfuscate the id and/or secret" means securely generating a randomized ID/secret. In this case, as long as DOT's default client ID/secret generation is sufficiently randomized, I think it is opening the door to end-user mistakes without granting anybody any additional security or protection. I don't really see any positives from supporting this use case for this reason.
Not only does this sound like a bad default, it sounds like an intentionally bad default. I would simply use my fork over implementing this. I think a better way would be to put these changes (if accepted) into a new major release and document the change for users. Projects release backwards-incompatible major versions all the time, and this is no different.
100% understandable. Would a separate, optional (and enabled by default),
Sort of. The application update page just shows (assuming I can tweak the changes to the application update view to show the client secret if I think there is room for improvement and compromise here, just let me know what you'd like me to do. |
Generally we want to give our users a deprecation release with a deprecation warning for any defaults that are changing. We try not to releases changes to defaults without prior notification.... It would make sense to add this as a feature within this major release and preserve the current behavior adding a deprecation warning for the upcoming change in the default. Then we can update the default in the next major release.
I think we have two use cases to consider. The admin use case where the admin either manages a public IDP that allows users to setup their own application or its a closed IDP managed by an admin... As one of those admins, I'd like to be able to explicitly edit the client id and secret. It is not an uncommon requirement for development (development envs), migrations, and support scenarios where users have lost a critical bit of info. Then we have the use case of a user who has self registered an application who we're trying to protect from making mistakes... It's not uncommon to have both types of users. I feel like this should be a permissions that varies what the user can do in the admin view...
As an admin who forgot which of the client secrets in my password database is the current one, I may want to rehash them to verify I have the correct match without needing to deploy a test RP, or possibly deploy an incorrect secret into an environment to test... It's not a security issue and I don't feel it's icky, but maybe GH's approach here of showing the last few chars could be a better replacement, but I feel like that is follow on issue.
The only IDP I work with that doesn't let you see the client secret is GH... Auth0, Google Cloud, KeyCloak, so I suggest we keep it visible when hash_client_secret is false. Which OAuth recommendation are you referring to? I think we need to review what use cases and security concerns we're addressing by hashing and preventing editing secrets before changing this behavior. We should have clearly defined cases we're protecting against to make sure we're not just blindly running down the alley of security by obscurity. |
The last section of the OAuth.com page that I referenced earlier.
I agree. That's why I specifically called out that displaying the raw hashed value is not a security issue AFAICT. I assume that OAuth.com is at least a somewhat authoritative source about OAuth, so I would not consider following their recommendation to display the secret only once or to reauthenticate before displaying the secret value to be security through obscurity. Forcing a fresh reauthentication helps minimize the blast radius if an application developer account gets compromised, and requiring user interaction to specifically show the secret helps stymie anybody who is shoulder surfing. Both of which I would consider defense in depth. Thank you for your responses here and on this project more generally, I really appreciate your time and effort. It sounds like there are intended use cases that conflict with this PR. Forcing fresh reauthentication before displaying the client secret and/or requiring a user click a button to load or show the client secret might be best left for a different author in a different PR. |
so you're referring to
The language uses should with regard to hashing and also shows options for not hashing where it is hidden in the ui and only revealed after clicking to display which can also be combined with an auth refresh. We offer both options hashed and unhashed secrets to our users. I would support the hashed presentation updates if we still show a partial of the secret. I would also support a show link with a re-auth to display the secret in the cases where it is not hashed. I wasn't trying to shut you down. We just need to talk through these things to make sure we're doing something that works for the module. I think your headed in roughly the right direction, there are just some Ts to cross and Is to dot. |
For hashed client secrets, I cannot see a way to implement that without storing a part of the cleartext secret next to or as part of the raw hash+algorithm+iterations+salt in the database. Which means that we wouldn't be able to directly farm the secret handling out to Django's password handling functions like we do now. We would either need to wrap Django's functionality or implement our own. I don't like either of those options. Am I missing something? Happy to be shown a good way to do this. 😄
👍
Oh, all good. Sorry if I came across like that, I just don't think this PR is going to support the use cases you have presented, and I think we've reached the point where this PR is too different than where we're heading and expect to end up to keep this PR around in an open yet unmerged state. I have briefly looked around for a third party app or something to help reauth an already-logged-in user, and I haven't found anything. 😕 I think the next step is to kinda prototype and see how difficult it would be to do that ourselves, or if we want to do so in a separate Django app and depend on that one. I cannot commit to that for the foreseeable future though. |
We hash the client secret in oauth2_provider.models.ClientSecretField.pre_save. This is the client_secet field on AbstractApplication. I would modify abstract application to have a client_secret_hint text field that contained the last few characters in the secret then keep it in sync with the hashed version. I bet you could override the .save method on AbstractApplication or somewhere similar to get at the prehashed secret and update the hint. In the UI we would probably only want to show the hint if hashing was enabled, but we should always maintain the client_secret_hint so if someone enables hashing it will already be populated.
I was worried that I'd discouraged you. I definitely want to see stuff like this land in DOT.
We don't necessarily need the re-auth now. An initial implementation could just be click to reveal.. That would be enough to prevent casual shoulder surfing and set us up to add re-auth later. |
Description of the Change
Instead of showing the hashed value for client secret, this PR removes the value from the update form and displays the client secret only one time in a Django flash message.
This adds
django.contrib.messages
as a dependency, but I think that's a reasonable tradeoff.Additionally, this removes the client ID and client secret from the generated form in the application registration view (which disallows users from selecting their own arbitrary client ID), and also removes the client ID, client secret, and
hash_client_secret
boolean/checkbox from the generated form in the application update view. The client ID is still rendered as a non-field value. This allows users to override the HTML templates if they don't want to display the client ID on the application details page.Checklist
CHANGELOG.md
updated (only for user relevant changes)AUTHORS