Skip to content
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

Send client_id for OAuth2 registry auth #1546

Merged
merged 3 commits into from
Mar 12, 2019
Merged

Conversation

chanseokoh
Copy link
Member

Fixes #1545. client_id is a required parameter.

TadCordle
TadCordle previously approved these changes Mar 11, 2019
Copy link
Member

@loosebazooka loosebazooka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a situation where this needs to be configured? Or would the server just include "jib" as a valid client?

@chanseokoh
Copy link
Member Author

My understanding is that this OAuth2 Client ID is a public identifier for an app. In our case, the app is Jib. Basically, I think we can generate whatever value we want. The spec doc says the auth server does not need to have ID registered, so I think any value will work.

My only concern is that, although the ID is public, this doc says it's a good practice to use an ID that is not easily guessable. I think I'll generate some longer string.

@loosebazooka
Copy link
Member

It's be hard to make an unguessable client ID of we hardcore it. If we need to make it configurable in the future maybe we can deal with that then.

@chanseokoh
Copy link
Member Author

chanseokoh commented Mar 11, 2019

Let me use an MD5 hash da031fe481a93ac107a95a96462358f9 generated from a string jib. (https://www.md5hashgenerator.com/) The ID is public, so it really shouldn't matter whether it's known. It is just that, it will be a slight deterrent if using a non-trivial ID. Like, if someone already knows an email address of someone else, it is slightly easier to attempt logging in by trying a brute-force password attack. And I think it's not really something that users want to configure.

@briandealwis
Copy link
Member

Since it's likely the client-id will appear in the registry logs, IMHO we should use something that clearly indicates Jib — the TOOL_NAME would be best, wouldn't it?

@TadCordle TadCordle dismissed their stale review March 11, 2019 20:13

Further discussion to be had

@chanseokoh
Copy link
Member Author

Good idea. The class is in jib-core, so the TOOL_NAME is jib-core. And I think it's not worth passing down jib-maven-plugin or jib-gradle-plugin depending on the execution context. So I think I'd just have jib as a prefix: jib.da031fe481a93ac107a95a96462358f9. It's clear from the ID that the application is Jib, and as suggested in the doc above, the entire ID is not able to guess without any context.

@briandealwis
Copy link
Member

Containerizer and BuildConfiguration provide getToolName(), but that isn't propagated into RegistryAuthenticator. We can revisit this if a need arises.

@chanseokoh chanseokoh merged commit 0f94475 into master Mar 12, 2019
@chanseokoh chanseokoh deleted the i1545-oauth-login-client_id branch March 12, 2019 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants