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

Allow null name when deserialising API key document #59485

Merged
merged 1 commit into from
Jul 14, 2020

Conversation

ywangd
Copy link
Member

@ywangd ywangd commented Jul 14, 2020

API keys can be created without names using grant API key action. This is considered as a bug (#59484). Since the feature has already been released, we need to accomodate existing keys that are created with null names. This PR relaxes the parser logic so that a null name is accepted.

Resolves: #59481

@ywangd ywangd added >test Issues or PRs that are addressing/adding tests :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v8.0.0 v7.9.0 labels Jul 14, 2020
@ywangd ywangd requested a review from tvernum July 14, 2020 01:15
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security (:Security/Authentication)

@elasticmachine elasticmachine added the Team:Security Meta label for security team label Jul 14, 2020
int version,
@Nullable BytesReference roleDescriptorsBytes,
BytesReference roleDescriptorsBytes,
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed Nullable and replaced optionalConstructorArg() with constructorArg() for hash and roleDescriptorsBytes fields for clarity:

  • The parser does not allow them to be null without something like builder.declareStringOrNull(...). Therefore the Nullable etc does not have any effects anyway. In another word, the overall code logic is not impacted.
  • Based on the logic in ApiKeyService#newDocument, these two fields can never be null.

Copy link
Contributor

@tvernum tvernum left a comment

Choose a reason for hiding this comment

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

LGTM

@ywangd ywangd merged commit c0fc9d0 into elastic:master Jul 14, 2020
ywangd added a commit to ywangd/elasticsearch that referenced this pull request Jul 14, 2020
API keys can be created without names using grant API key action. This is considered as a bug (elastic#59484). Since the feature has already been released, we need to accomodate existing keys that are created with null names. This PR relaxes the parser logic so that a null name is accepted.
ywangd added a commit that referenced this pull request Jul 14, 2020
API keys can be created without names using grant API key action. This is considered as a bug (#59484). Since the feature has already been released, we need to accomodate existing keys that are created with null names. This PR relaxes the parser logic so that a null name is accepted.
@albertzaharovits
Copy link
Contributor

Auditing also doesn't expect API keys with null names 😢 . I'll raise a fix.

@ywangd
Copy link
Member Author

ywangd commented Jul 14, 2020

Auditing also doesn't expect API keys with null names 😢 . I'll raise a fix.

Good catch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) Team:Security Meta label for security team >test Issues or PRs that are addressing/adding tests v7.9.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression when an API key has no name
5 participants