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

[Feature/Identity] Allow for Encryption/Decryption of Principals into PITs #4730

Merged

Conversation

stephen-crawford
Copy link
Contributor

@stephen-crawford stephen-crawford commented Oct 10, 2022

Changes in the ExtensionTokenProcessor class to allow for encryption & decryption of principal & extension identifiers.

Tests not yet added but will add unit tests. Right now there are some lingering issues with the code that I would appreciate feedback or thoughts on:

As you can see at Line 146, the ExtractPrincipal method attempts to return the Principal object that is extracted from the encrypted message. Right now, I am not sure that we are actually storing the Principals themselves in any way so to be able to return an entire Principal instance we would need to be able to store and then retrieve the instance via NAME lookup.

I am also not entirely convinced by my implementation of the encoding combining at Line 106. I know you can combine byte[]s in this manner but I have not had a chance to confirm this preserves the encoding integrity as expected.

Resolves

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@stephen-crawford stephen-crawford requested review from a team and reta as code owners October 10, 2022 21:00
@stephen-crawford stephen-crawford added the feedback needed Issue or PR needs feedback label Oct 10, 2022
Signed-off-by: Stephen Crawford <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@cwperks
Copy link
Member

cwperks commented Oct 11, 2022

@scrawfor99 Can you prepend the title of this PR with [Feature/Identity]? It makes it easier to find PRs against this feature branch and core maintainers can see from the description that its for the feature branch.

@stephen-crawford stephen-crawford changed the title Allow for Encryption/Decryption of Principals into PITs [Feature/Identity] Allow for Encryption/Decryption of Principals into PITs Oct 11, 2022
@stephen-crawford
Copy link
Contributor Author

@scrawfor99 Can you prepend the title of this PR with [Feature/Identity]? It makes it easier to find PRs against this feature branch and core maintainers can see from the description that its for the feature branch.

Should be all set

Signed-off-by: Stephen Crawford <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Copy link
Member

@cwperks cwperks left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @scrawfor99! If you want to submit a PR to solicit initial feedback you can use Github's Draft PR feature to distinguish the PR as a WIP (work in progress).

Can you add tests for the functionality added in this PR?

@stephen-crawford
Copy link
Contributor Author

stephen-crawford commented Oct 11, 2022

Thank you for the PR @scrawfor99! If you want to submit a PR to solicit initial feedback you can use Github's Draft PR feature to distinguish the PR as a WIP (work in progress).

Oops I did not realize but will mark things as such in the future--still new to using GitHub with more than 1/2 other people haha.

Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: null ❌
  • URL: null
  • CommitID: c0144c1

Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Stephen Crawford <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Stephen Crawford <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Stephen Crawford <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@stephen-crawford
Copy link
Contributor Author

@peternied What is the protocol for handling this? Everything seems fine on my end but now the check is failing because of memory errors from other code. Unless I am misunderstanding the console output.

@peternied
Copy link
Member

Everything seems fine on my end but now the check is failing because of memory errors from other code.

Sounds like there is as bug in OpenSearch, can you capture the error output and create a bug like the following?

@stephen-crawford
Copy link
Contributor Author

Everything seems fine on my end but now the check is failing because of memory errors from other code.

Sounds like there is as bug in OpenSearch, can you capture the error output and create a bug like the following?

* [[BUG] flaky test `index/80_geo_point/Single point test` #4852](https://github.com/opensearch-project/OpenSearch/issues/4852)

Done so awaiting response #4907

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Stephen Crawford <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@peternied peternied merged commit d9b5059 into opensearch-project:feature/identity Oct 26, 2022
@peternied
Copy link
Member

@scrawfor99 I merged this because the failed tests are unrelated to your changes. Next merge from main should resolve them (also what a pain!) Thanks for your contribution and stick-with-it-ness on this one!

peternied pushed a commit that referenced this pull request Nov 2, 2022
… PITs (#4730)

Allow for Encryption/Decryption of Principals into PITs

Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback needed Issue or PR needs feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants