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

[BUG] Are ldaptive and the Pattern classes needed as part of SAFE_CLASSES list in Base64Helper? #2800

Open
parasjain1 opened this issue May 29, 2023 · 2 comments
Labels
bug Something isn't working triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.

Comments

@parasjain1
Copy link
Contributor

parasjain1 commented May 29, 2023

What is the bug?
Why are ldaptive classes (LdapEntry, SearchEntry, AbstractLdapBean, LdapAttribute), Pattern class part of the SAFE_CLASSES list in Base64Helper? (source code).

Additional Context

While working on #2780 for a faster serialization protocol in Security Plugin, I encountered a doubt around whether the above mentioned classes are needed as part of SAFE_CLASSES list in Base64Helper?

I tracked all usages of Base64Helper::serializeObject and Base64Helper::deserializeObject and found only the following classes to be directly serialized/deserialized in security plugin -

  • String
  • UnmodifiableMap (for DLS queries, Masked Fields, FLS Fields)
  • SourceFieldsContext
  • User
  • LdapUser
  • InjectedUser
  • InetSocketAddress

To track nested usages of ldaptive classes and the Pattern class for the above classes, found that they are only used with the LdapUser (which extends from User). Below are few observations that I noted -

  • The LdapEntry userEntry field of LdapUser is transient and not serialized. The comment on User::getUserEntry specifies this clearly, deserialized LdapUser will have null value for userEntry.

  • LdapAuthenticationBackend has the functionality to construct LdapEntry and LdapAttribute for the user and takes care of populating the user attributes map here.

  • LdapAttribute too is not involved in serialization as the value of LdapAttribute::getName() is used to populate the User::attributes map (source code)

  • I removed the all of the mentioned classes from SAFE_CLASSES list and could (de)serialize LdapUser without any issues.

The above observations lead me to the conclusion that these classes are NOT needed to be listed as SAFE_CLASSES. Starting this issue to understand the reasoning behind the presence of these classes in SAFE_CLASSES, in case they indeed are to be present, otherwise, get a confirmation on the above pointers and conclude that their removal from SAFE_CLASSES will not break anything.

@parasjain1 parasjain1 added bug Something isn't working untriaged Require the attention of the repository maintainers and may need to be prioritized labels May 29, 2023
@parasjain1
Copy link
Contributor Author

parasjain1 commented May 29, 2023

Here are the changes that remove the above mentioned classes from SAFE_CLASSES along with a couple of additional tests for Base64Helper.
main...parasjain1:opensearch-security:parasjaz-safe-class-removal

@davidlago davidlago added triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable. and removed untriaged Require the attention of the repository maintainers and may need to be prioritized labels May 31, 2023
@nibix
Copy link
Collaborator

nibix commented Jun 5, 2023

@parasjain1

The above observations lead me to the conclusion that these classes are NOT needed to be listed as SAFE_CLASSES.

I am sharing this conclusion, it looks indeed like these classes are not needed there :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.
Projects
None yet
Development

No branches or pull requests

3 participants