Move secret validation logic and authN-loadPrincipal into PolarisSecretsManager
#511
Conversation
snazy
requested review from
jbonofre,
ashvina,
RussellSpitzer,
vvcephei,
takidau,
jackye1995,
flyrain,
eric-maynard,
collado-mike,
ebyhr and
adutra
as code owners
| @Nonnull | ||
| EntityResult loadPrincipal( | ||
| @Nonnull PolarisCallContext callCtx, | ||
| @Nullable String roleName, | ||
| @Nullable String clientId, | ||
| @Nullable Long principalId); |
There was a problem hiding this 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
There was a problem hiding this 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.
There was a problem hiding this 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.
There was a problem hiding this 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.
There was a problem hiding this 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.
There was a problem hiding this 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
| public PolarisBaseEntity getPrincipal() { | ||
| return principal; | ||
| } |
There was a problem hiding this 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.
snazy
force-pushed
the
move-secret-validation-logic
branch
from
226558f to
1686cea
Compare
…retsManager` 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`.
snazy
force-pushed
the
move-secret-validation-logic
branch
from
1686cea to
46c7647
Compare
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.