Decode URL's user info before setting auth header #760

i7c · 2018-03-20T12:16:47Z

Commit 789621d added possibility to set http basic auth header in the
schema registry client by putting them on the URL (as in
https://user:password@host:port). However, if the credentials contain
special characters, they must be percent-encoded:

https://tools.ietf.org/html/rfc3986#section-2.1

This patch makes schema registry client respect percent encoded
credentials by decoding them before calculating the base64
representation. Please note, that the decoding step happens
independently of the specific credentials provider, so other ways to
supply credentials might be affected as well.
Currently, all ways to set credentials use the user info form
"user:pass", so this should not be harmful.

ghost · 2018-03-20T12:16:49Z

It looks like @i7c hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement here.

Once you've signed reply with [clabot:check] to prove it.

Appreciation of efforts,

clabot

i7c · 2018-03-20T12:18:23Z

Please note, that if merged, you also need to percent-encode credentials set via 'schema.registry.basic.auth.user.info'. If this is not wanted, we should maybe consider using completely different means to set the credentials (independetly of any "user info" form).

i7c · 2018-03-20T12:22:21Z

[clabot:check]

ghost · 2018-03-20T12:22:22Z

@confluentinc It looks like @i7c just signed our Contributor License Agreement. 👍

Always at your service,

clabot

codyaray

nice work! thanks for the PR.

just a couple small things...

codyaray · 2018-03-27T15:42:48Z

client/src/main/java/io/confluent/kafka/schemaregistry/client/rest/RestService.java

+        String authHeader = DatatypeConverter.printBase64Binary(userInfoBytes);
+        connection.setRequestProperty("Authorization", "Basic " + authHeader);
+      } catch (UnsupportedEncodingException e) {
+        return;


We shouldn't swallow this exception. Looks like we could wrap it in an IllegalArgumentException so it gets passed back to the user (like in parseBaseUrl).

Yeah, I wouldn't wrap the lines here in a try-catch block. We want to indicate to the calling methods that an error happened.

Uhm, I’ll gladly change it, but the UnsupportedEncodingException is thrown, if "UTF-8" is not supported by the stdlib/jvm as possible encoding, so IllegalArgumentException seems like an odd choice to me. I think usually one would throw an AssertionError in this case. Whether the encoding is supported or not depends on the environment rather than what the user provides as data.

Good point, I'm less opinionated on the exception thrown or whether we log something. Just know it'll be a pain for people to troubleshoot if we silently swallow it. @wicknicks ?

Yes @codyaray, definitely agree that we shouldn't simply swallow the exception here. Good point, @i7c! Also, since this method is called setBasicAuthRequestHeader, I am less inclined to do the decoding here. The more appropriate place would be just before calling this method on line 157 in this class, and have the setter only set stuff, something like this:

if (basicAuthCredentialProvider != null) { userInfo = basicAuthCredentialProvider.getUserInfo(connection.getURL())) userInfoBytes = URLDecoder.decode(userInfo, "UTF-8").getBytes(StandardCharsets.UTF_8); setBasicAuthRequestHeader(userInfoBytes, connection); }

This would also allow easier handling of the IOException. The other alternative is to have setBasicAuthRequestHeader throw an IOException, which is a bit odd as well.

First of all thanks for the helpful feedback. I get the points and I agree with what you said.

However, trying out your proposal bumped up cyclomatic and NPath complexity of sendHttpRequest() above the allowed limits, which made me think: Why should this method (or the setter) have to deal with decoding user input anyways?

Wouldn't it make more sense to have the CredentialProviders deal with decoding? This would have some nice properties:

we can decode only once (when BasicAuthCredentialProvider.config() is called or in a "caching manner" in case of the UrlBasicAuthCredentialProvider), so we reduce execution time of sendHttpRequest()

we can deal with errors where they occur conceptually, in "providing credentials"

we can offer a nicer way for the user to specify credentials (with a new Provider implementation), namely with two properties "username" and "password" where no URL encoding is necessary. I actually don't like the idea that the user has to put something like johndoe:my%20secret in there anyways. It also might cause some hassle when using configuration management. Think of a centralised store for secrets, so configuration management would need to URL-encode the provided secret first ...)

Please let me know what you think about it and if this is getting too big for a simple feature. ;)
I’ll gladly provide a patch, should you like the idea. 😄

I like the direction you're thinking in. We actually already do support passing user and password separately. Seems like it may just be undocumented/poorly documented. So I'm not sure how far down this road with code refactoring versus docs improvements.

Here's a gist that does SR auth using system properties.
https://gist.github.com/mageshn/c623f2d43b66b1153f5da32662ca5e94

(Sorry if this possibility was already obvious to you. This isn't a code base I normally touch.)

@i7c: thanks a lot for your thoughts on this. Would love that patch. Thank you!

Some thoughts:

I like the new contract being established in your second suggestion.

Also, maybe we can create an abstract class which extends BasicAuthCredentialProvider to house your suggestions. The other Provider implementations would just extend this new class. Once we officially move to Java 8, this could be replaced with a default method in the interface.

codyaray · 2018-03-27T15:47:54Z

client/src/main/java/io/confluent/kafka/schemaregistry/client/rest/RestService.java

-      connection.setRequestProperty("Authorization", "Basic " + authHeader);
+      try {
+        byte[] userInfoBytes =
+          URLDecoder.decode(userInfo, "UTF-8").getBytes(StandardCharsets.UTF_8);


We should add some test cases to make our intention clear here.

https://github.com/i7c/schema-registry/blob/master/client/src/test/java/io/confluent/kafka/schemaregistry/client/rest/RestServiceTest.java#L85

Might be easier to make this method @VisibleForTesting than try to implicitly test it like the above does though.

wicknicks

Thanks for the changes. Much appreciated!

wicknicks · 2018-03-27T16:45:18Z

client/src/main/java/io/confluent/kafka/schemaregistry/client/rest/RestService.java

+        String authHeader = DatatypeConverter.printBase64Binary(userInfoBytes);
+        connection.setRequestProperty("Authorization", "Basic " + authHeader);
+      } catch (UnsupportedEncodingException e) {
+        return;


Yeah, I wouldn't wrap the lines here in a try-catch block. We want to indicate to the calling methods that an error happened.

i7c · 2018-04-09T22:58:51Z

I can hopefully wrap this up tomorrow and we can do another round of review. :)

This commit introduces a common superclass AbstractBasicAuthCredentialProvider, that may house useful methods for the inner workings of the concrete implementations of BasicAuthCredentialProvider. decodeUserInfo() can be used to decode percent-encoded credentials: https://tools.ietf.org/html/rfc3986#section-2.1 UrlBasicAuthCredentialProvider and UserInfoCredentialProvider both use decodeUserInfo() to handle encoded credentials with special characters correctly.

This commit adds a convenience method to BasicAuthCredentialProvider's interface: getAuthHeader() returns the base64-encoded auth header derived from the provided credentials, so the calling site does not have to bother about encoding of credentials.

We introduce a new credential provider that is selected by using the credential source "USERNAME_PASSWORD". The user must supply two options, namely schema.registry.basic.auth.username and schema.registry.basic.auth.password. The motivation for this additional way is, that the user can provide the credentials in plain text, which may be more convenient than percent-encoded credentials as part of the target URL.

i7c · 2018-04-11T19:50:02Z

Hi again,

my reworked pull request is out for discussion. I split it in three commits, because I’m doing three steps which you may individually decide if you like them or not.

the common base class and actual URL decoding, which was the main concern for the original PR
providing the auth header directly from the credential provider. I’m not sure if this is a good way to go, but since the providers are only meant for basic auth, I do not see any harm at least.
another way of specifying credentials without URL encoding

I have not yet tested the code against an actual schema registry. I will do so if you think the patch series is going in an acceptable direction.

Looking forward to getting feedback! :)

mageshn · 2018-11-09T00:48:49Z

@i7c schema.registry.basic.auth.user.info is part of the configs. So, I don't see a need to decode the credentials there. The only place we potentially might need decoding is when the credentials are provided via the URL config itself. In which case, we should just be able to do the decode just in the UrlBasicAuthCredentialProvider. WDYT?

cla-assistant · 2023-09-25T16:24:43Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

i7c mentioned this pull request Mar 20, 2018

Support basic auth credentials embedded in the schema.registry.url #571

Merged

codyaray requested changes Mar 27, 2018

View reviewed changes

codyaray requested a review from wicknicks March 27, 2018 15:49

wicknicks reviewed Mar 27, 2018

View reviewed changes

i7c force-pushed the decode-user-info branch from a891dc8 to 53661fb Compare April 9, 2018 22:56

i7c added 3 commits April 11, 2018 21:24

i7c force-pushed the decode-user-info branch from 53661fb to b7700f6 Compare April 11, 2018 19:44

wicknicks requested a review from mageshn April 12, 2018 17:05

mageshn removed their request for review June 8, 2021 17:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decode URL's user info before setting auth header #760

Decode URL's user info before setting auth header #760

i7c commented Mar 20, 2018

ghost commented Mar 20, 2018

i7c commented Mar 20, 2018

i7c commented Mar 20, 2018

ghost commented Mar 20, 2018

codyaray left a comment

codyaray Mar 27, 2018

wicknicks Mar 27, 2018

i7c Mar 28, 2018 •

edited

Loading

codyaray Mar 28, 2018

wicknicks Mar 28, 2018

i7c Mar 30, 2018

codyaray Mar 30, 2018

wicknicks Apr 3, 2018 •

edited

Loading

codyaray Mar 27, 2018

wicknicks left a comment

wicknicks Mar 27, 2018

i7c commented Apr 9, 2018

i7c commented Apr 11, 2018

mageshn commented Nov 9, 2018

cla-assistant bot commented Sep 25, 2023

Decode URL's user info before setting auth header #760

Are you sure you want to change the base?

Decode URL's user info before setting auth header #760

Conversation

i7c commented Mar 20, 2018

ghost commented Mar 20, 2018

i7c commented Mar 20, 2018

i7c commented Mar 20, 2018

ghost commented Mar 20, 2018

codyaray left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

i7c Mar 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wicknicks Apr 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wicknicks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

i7c commented Apr 9, 2018

i7c commented Apr 11, 2018

mageshn commented Nov 9, 2018

cla-assistant bot commented Sep 25, 2023

i7c Mar 28, 2018 •

edited

Loading

wicknicks Apr 3, 2018 •

edited

Loading