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

Secret type to represent secrets and integrate with the secrets handling #1232

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

radcortez
Copy link
Member

@radcortez radcortez commented Oct 2, 2024

Adds a new wrapper type, Secret<T>, to represent a secret. When a Secret is part of a mapping, the path is added to the Secrets interceptor, to not include the secret in property names or access the secret without calling SecretKeys.doUnlocked.

This is initial work to support more advanced scenarios around secrets, that we should discuss.

@radcortez radcortez requested a review from dmlloyd October 2, 2024 13:09
@dmlloyd
Copy link
Contributor

dmlloyd commented Oct 2, 2024

The type is Secret but it's really a text password. Credentials however can be a variety of things - Key and its subclasses for example. Also we have a Password type and hierarchy in wildfly-elytron, which we use for security functions in Quarkus. This type extends Key, which is really the canonical supertype for secrets AFAICT.

What about supporting the existing types as secrets? Or at most maybe adding a wrapper e.g. Secret<PrivateKey> where we have a nested conversion process?

Also please look into how Elytron does password encoding because it could be relevant.

@radcortez
Copy link
Member Author

The type is Secret but it's really a text password. Credentials however can be a variety of things - Key and its subclasses for example. Also we have a Password type and hierarchy in wildfly-elytron, which we use for security functions in Quarkus.

Yes, I was not too certain about the name, but the rationale was to keep it consistent with what we already have, SecretKeys, SecretKeysHandler.

This type extends Key, which is really the canonical supertype for secrets AFAICT.

What do you mean? This only extends Destroyable, which, yes, I agree, is mainly for Key. The goal was to have a way to remove values from the mapping, because values are cached. Still, was only an idea.

What about supporting the existing types as secrets? Or at most maybe adding a wrapper e.g. Secret<PrivateKey> where we have a nested conversion process?

We need the mapping generator to identify which elements are considered secrets. Either we provide a list of types for that case, or maybe we can make it work generically with a wrapper type. Let me see what I can do.

}

@Override
public ConfigValue getValue(final ConfigSourceInterceptorContext context, final String name) {
if (SecretKeys.isLocked() && secrets.contains(name)) {
if (SecretKeys.isLocked() && secrets.contains(new PropertyName(name))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Creating a new PropertyTime every time getValue is called may impact performance, perhaps consider iterating the Set and comparing the name instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was to move forward with the implementation. I haven't profiled the code yet, but I am not too worried about it because when we are populating the config, the secrets are unlocked, but I'll have a look. Thanks!

@radcortez
Copy link
Member Author

I've moved Secret to a wrapper type Secret<T>, so this will allow using whatever type you want for the actual secret. The wrapper type only marks the property as a secret, which can be hidden from the list of names or forbid it to retrieve it directly with the API.

We need to figure out how to handle the nested converters piece. I'm wondering if we should support nested converter creation automatically.

@radcortez
Copy link
Member Author

//cc @sberyozkin @gsmet @yrodiere

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide Types to represent Secrets
3 participants