You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/05/29 20:31:44 UTC
[GitHub] [iceberg] rdblue opened a new pull request, #4896: Core: Add Basic auth for client credentials flow
rdblue opened a new pull request, #4896:
URL: https://github.com/apache/iceberg/pull/4896
This adds a Basic auth header to client credentials request as recommended in [RFC 6749 Section 2.3.1](https://datatracker.ietf.org/doc/html/rfc6749#section-2.3.1).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] bryanck commented on a diff in pull request #4896: Core: Add Basic auth for client credentials flow
Posted by GitBox <gi...@apache.org>.
bryanck commented on code in PR #4896:
URL: https://github.com/apache/iceberg/pull/4896#discussion_r884344954
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java:
##########
@@ -144,8 +148,19 @@ public static OAuthTokenResponse fetchToken(RESTClient client, Map<String, Strin
Map<String, String> request = clientCredentialsRequest(
credential, scope != null ? ImmutableList.of(scope) : ImmutableList.of());
+ Map<String, String> requestHeaders;
+ if (headers.containsKey(AUTHORIZATION_HEADER)) {
+ requestHeaders = headers;
+ } else {
+ String credentialAsBase64 = Base64.getEncoder().encodeToString(credential.getBytes(StandardCharsets.UTF_8));
+ requestHeaders = ImmutableMap.<String, String>builder()
+ .putAll(headers)
+ .put(AUTHORIZATION_HEADER, BASIC_PREFIX + credentialAsBase64)
Review Comment:
I see, thanks for the context. If you need to keep it in the payload anyway, then I'd vote to not add the header also. The spec wording seemed more about avoiding putting it in the payload.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] bryanck commented on a diff in pull request #4896: Core: Add Basic auth for client credentials flow
Posted by GitBox <gi...@apache.org>.
bryanck commented on code in PR #4896:
URL: https://github.com/apache/iceberg/pull/4896#discussion_r884329312
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java:
##########
@@ -144,8 +148,19 @@ public static OAuthTokenResponse fetchToken(RESTClient client, Map<String, Strin
Map<String, String> request = clientCredentialsRequest(
credential, scope != null ? ImmutableList.of(scope) : ImmutableList.of());
+ Map<String, String> requestHeaders;
+ if (headers.containsKey(AUTHORIZATION_HEADER)) {
+ requestHeaders = headers;
+ } else {
+ String credentialAsBase64 = Base64.getEncoder().encodeToString(credential.getBytes(StandardCharsets.UTF_8));
+ requestHeaders = ImmutableMap.<String, String>builder()
+ .putAll(headers)
+ .put(AUTHORIZATION_HEADER, BASIC_PREFIX + credentialAsBase64)
Review Comment:
It seems like this will put the credential as a basic auth header, but `clientCredentialsRequest()` already puts it in the payload so it is really needed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue closed pull request #4896: Core: Add Basic auth for client credentials flow
Posted by GitBox <gi...@apache.org>.
rdblue closed pull request #4896: Core: Add Basic auth for client credentials flow
URL: https://github.com/apache/iceberg/pull/4896
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #4896: Core: Add Basic auth for client credentials flow
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #4896:
URL: https://github.com/apache/iceberg/pull/4896#issuecomment-1140520132
@bryanck, can you take a look? This adds the Basic authentication header for client credentials exchanges.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #4896: Core: Add Basic auth for client credentials flow
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4896:
URL: https://github.com/apache/iceberg/pull/4896#discussion_r884331728
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java:
##########
@@ -144,8 +148,19 @@ public static OAuthTokenResponse fetchToken(RESTClient client, Map<String, Strin
Map<String, String> request = clientCredentialsRequest(
credential, scope != null ? ImmutableList.of(scope) : ImmutableList.of());
+ Map<String, String> requestHeaders;
+ if (headers.containsKey(AUTHORIZATION_HEADER)) {
+ requestHeaders = headers;
+ } else {
+ String credentialAsBase64 = Base64.getEncoder().encodeToString(credential.getBytes(StandardCharsets.UTF_8));
+ requestHeaders = ImmutableMap.<String, String>builder()
+ .putAll(headers)
+ .put(AUTHORIZATION_HEADER, BASIC_PREFIX + credentialAsBase64)
Review Comment:
I thought that you said we preferred to use the Basic header, so I added it. I didn't remove the ID and secret from the payload because it is used in some cases, like when there is a Bearer token that needs to be passed in Authorization for the catalog. We can do whatever you think is right here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] bryanck commented on a diff in pull request #4896: Core: Add Basic auth for client credentials flow
Posted by GitBox <gi...@apache.org>.
bryanck commented on code in PR #4896:
URL: https://github.com/apache/iceberg/pull/4896#discussion_r884329312
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java:
##########
@@ -144,8 +148,19 @@ public static OAuthTokenResponse fetchToken(RESTClient client, Map<String, Strin
Map<String, String> request = clientCredentialsRequest(
credential, scope != null ? ImmutableList.of(scope) : ImmutableList.of());
+ Map<String, String> requestHeaders;
+ if (headers.containsKey(AUTHORIZATION_HEADER)) {
+ requestHeaders = headers;
+ } else {
+ String credentialAsBase64 = Base64.getEncoder().encodeToString(credential.getBytes(StandardCharsets.UTF_8));
+ requestHeaders = ImmutableMap.<String, String>builder()
+ .putAll(headers)
+ .put(AUTHORIZATION_HEADER, BASIC_PREFIX + credentialAsBase64)
Review Comment:
This will put the credential as a basic auth header, but `clientCredentialsRequest()` already puts it in the payload so it is really needed here? We just need it in one or the other per the spec IIRC.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] bryanck commented on a diff in pull request #4896: Core: Add Basic auth for client credentials flow
Posted by GitBox <gi...@apache.org>.
bryanck commented on code in PR #4896:
URL: https://github.com/apache/iceberg/pull/4896#discussion_r884329312
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java:
##########
@@ -144,8 +148,19 @@ public static OAuthTokenResponse fetchToken(RESTClient client, Map<String, Strin
Map<String, String> request = clientCredentialsRequest(
credential, scope != null ? ImmutableList.of(scope) : ImmutableList.of());
+ Map<String, String> requestHeaders;
+ if (headers.containsKey(AUTHORIZATION_HEADER)) {
+ requestHeaders = headers;
+ } else {
+ String credentialAsBase64 = Base64.getEncoder().encodeToString(credential.getBytes(StandardCharsets.UTF_8));
+ requestHeaders = ImmutableMap.<String, String>builder()
+ .putAll(headers)
+ .put(AUTHORIZATION_HEADER, BASIC_PREFIX + credentialAsBase64)
Review Comment:
This will put the credential as a basic auth header, but `clientCredentialsRequest()` already puts it in the payload so it is really needed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org