You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "snazy (via GitHub)" <gi...@apache.org> on 2024/12/09 13:32:42 UTC
[PR] Move secret validation logic and authN-loadPrincipal into `PolarisSecretsManager` [polaris]
snazy opened a new pull request, #511:
URL: https://github.com/apache/polaris/pull/511
The logic _how_ a principal and/or principal secret's are persisted should be transparent to the calling code. Relying on the persistence internals for principals and secrets management makes it impossible to factor out secrets management / make principal management possible.
This change moves the secret validation and retrieval of a principal by client-ID behind an implementation of `PolarisSecretsManager`.
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move secret validation logic and authN-loadPrincipal into `PolarisSecretsManager` [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy closed pull request #511: Move secret validation logic and authN-loadPrincipal into `PolarisSecretsManager`
URL: https://github.com/apache/polaris/pull/511
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move secret validation logic and authN-loadPrincipal into `PolarisSecretsManager` [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #511:
URL: https://github.com/apache/polaris/pull/511#discussion_r1879429014
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisSecretsManager.java:
##########
@@ -39,6 +41,17 @@ public interface PolarisSecretsManager {
PrincipalSecretsResult loadPrincipalSecrets(
@Nonnull PolarisCallContext callCtx, @Nonnull String clientId);
+ @Nonnull
+ SecretValidationResult validateSecret(
+ @Nonnull PolarisCallContext callCtx, @Nonnull String clientId, @Nonnull String clientSecret);
+
+ @Nonnull
+ EntityResult loadPrincipal(
+ @Nonnull PolarisCallContext callCtx,
+ @Nullable String roleName,
+ @Nullable String clientId,
+ @Nullable Long principalId);
Review Comment:
I think, the assumption that Polaris is _the_ source-of-truth for principals is not correct. Principals/identities are usually managed elsewhere. It would be difficult to force people to add a principal-ID as a new attribute to new or existing identities. On top, principal-ID is an internal concern.
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move secret validation logic and authN-loadPrincipal into `PolarisSecretsManager` [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #511:
URL: https://github.com/apache/polaris/pull/511#discussion_r1881184533
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisSecretsManager.java:
##########
@@ -39,6 +41,17 @@ public interface PolarisSecretsManager {
PrincipalSecretsResult loadPrincipalSecrets(
@Nonnull PolarisCallContext callCtx, @Nonnull String clientId);
+ @Nonnull
+ SecretValidationResult validateSecret(
+ @Nonnull PolarisCallContext callCtx, @Nonnull String clientId, @Nonnull String clientSecret);
+
+ @Nonnull
+ EntityResult loadPrincipal(
+ @Nonnull PolarisCallContext callCtx,
+ @Nullable String roleName,
+ @Nullable String clientId,
+ @Nullable Long principalId);
Review Comment:
The secrets manager is only used for Polaris-managed Principals. There's no need to manage secrets for externally managed Principals. I think the assumption that Polaris is authoritative here is a safe one
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move secret validation logic and authN-loadPrincipal into `PolarisSecretsManager` [polaris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #511:
URL: https://github.com/apache/polaris/pull/511#issuecomment-2584998133
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move secret validation logic and authN-loadPrincipal into `PolarisSecretsManager` [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #511:
URL: https://github.com/apache/polaris/pull/511#discussion_r1876569716
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisSecretsManager.java:
##########
@@ -100,4 +113,34 @@ public PolarisPrincipalSecrets getPrincipalSecrets() {
return principalSecrets;
}
}
+
+ /** the result of load/rotate principal secrets */
+ class SecretValidationResult extends BaseResult {
+
+ private final PolarisBaseEntity principal;
+
+ public SecretValidationResult(
+ @Nonnull BaseResult.ReturnStatus errorCode, @Nullable String extraInformation) {
+ super(errorCode, extraInformation);
+ this.principal = null;
+ }
+
+ public SecretValidationResult(@Nonnull PolarisBaseEntity principal) {
+ super(BaseResult.ReturnStatus.SUCCESS);
+ this.principal = principal;
+ }
+
+ @JsonCreator
+ private SecretValidationResult(
+ @JsonProperty("returnStatus") @Nonnull BaseResult.ReturnStatus returnStatus,
+ @JsonProperty("extraInformation") @Nullable String extraInformation,
+ @JsonProperty("principalSecrets") @Nonnull PolarisBaseEntity principal) {
+ super(returnStatus, extraInformation);
+ this.principal = principal;
+ }
+
+ public PolarisBaseEntity getPrincipal() {
+ return principal;
+ }
Review Comment:
Same comment on returning the principal from the secret validation command. I can imagine storing extra metadata, such as the principal id with the client id / secret, but the entity itself should be returned from the persistence layer.
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisSecretsManager.java:
##########
@@ -39,6 +41,17 @@ public interface PolarisSecretsManager {
PrincipalSecretsResult loadPrincipalSecrets(
@Nonnull PolarisCallContext callCtx, @Nonnull String clientId);
+ @Nonnull
+ SecretValidationResult validateSecret(
+ @Nonnull PolarisCallContext callCtx, @Nonnull String clientId, @Nonnull String clientSecret);
+
+ @Nonnull
+ EntityResult loadPrincipal(
+ @Nonnull PolarisCallContext callCtx,
+ @Nullable String roleName,
+ @Nullable String clientId,
+ @Nullable Long principalId);
Review Comment:
Loading the principal entity should be the purview of the persistence layer, not the secrets manager. IMO, the secrets manager should be something like Vault or AWS Secrets manager that knows nothing about Polaris entities, but does know how to store client id / secret
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move secret validation logic and authN-loadPrincipal into `PolarisSecretsManager` [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #511:
URL: https://github.com/apache/polaris/pull/511#discussion_r1879198859
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisSecretsManager.java:
##########
@@ -39,6 +41,17 @@ public interface PolarisSecretsManager {
PrincipalSecretsResult loadPrincipalSecrets(
@Nonnull PolarisCallContext callCtx, @Nonnull String clientId);
+ @Nonnull
+ SecretValidationResult validateSecret(
+ @Nonnull PolarisCallContext callCtx, @Nonnull String clientId, @Nonnull String clientSecret);
+
+ @Nonnull
+ EntityResult loadPrincipal(
+ @Nonnull PolarisCallContext callCtx,
+ @Nullable String roleName,
+ @Nullable String clientId,
+ @Nullable Long principalId);
Review Comment:
Vault, Secrets Manager, and K8s all allow storing secret values as structured types, like JSON. I think it's pretty easy to imagine a layout like using the `clientId` as the secret key and storing the secret value as something like `{"secret": "abcd", "principalId": 1}`. This allows full decoupling of the PolarisSecretsManager from the PolarisMetaStoreManager, whereas returning the `PrincipalEntity` from the `PolarisSecretsManager` means it has to maintain a reference to the MetaStore and the MetaStore has to deal with lookup keys beside name and id. No other entity has a lookup key beside name or id, so I think keeping it consistent for Principal makes sense.
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move secret validation logic and authN-loadPrincipal into `PolarisSecretsManager` [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #511:
URL: https://github.com/apache/polaris/pull/511#discussion_r1879420822
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisSecretsManager.java:
##########
@@ -39,6 +41,17 @@ public interface PolarisSecretsManager {
PrincipalSecretsResult loadPrincipalSecrets(
@Nonnull PolarisCallContext callCtx, @Nonnull String clientId);
+ @Nonnull
+ SecretValidationResult validateSecret(
+ @Nonnull PolarisCallContext callCtx, @Nonnull String clientId, @Nonnull String clientSecret);
+
+ @Nonnull
+ EntityResult loadPrincipal(
+ @Nonnull PolarisCallContext callCtx,
+ @Nullable String roleName,
+ @Nullable String clientId,
+ @Nullable Long principalId);
Review Comment:
The issue here is: there are currently _two_ lookup keys: client-ID _and_ principal-ID. Client-ID is the natural key, so I think that we should only use that one.
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move secret validation logic and authN-loadPrincipal into `PolarisSecretsManager` [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #511:
URL: https://github.com/apache/polaris/pull/511#discussion_r1877566584
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisSecretsManager.java:
##########
@@ -39,6 +41,17 @@ public interface PolarisSecretsManager {
PrincipalSecretsResult loadPrincipalSecrets(
@Nonnull PolarisCallContext callCtx, @Nonnull String clientId);
+ @Nonnull
+ SecretValidationResult validateSecret(
+ @Nonnull PolarisCallContext callCtx, @Nonnull String clientId, @Nonnull String clientSecret);
+
+ @Nonnull
+ EntityResult loadPrincipal(
+ @Nonnull PolarisCallContext callCtx,
+ @Nullable String roleName,
+ @Nullable String clientId,
+ @Nullable Long principalId);
Review Comment:
The problem is: effectively only the secrets manager knows which attributes it has at hand, correct?
Currently, it's the principal-ID, which is another unique ID in the JWT - but it should rather be just the client-ID. But that's currently hard to refactor, because the data model details leak all the way up. That's why #512 is there as an immediate follow-up.
TL;DR this PR is one of a series of upcoming PRs to untangle the hard dependency of all the services on the data model details/internals.
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org