Skip to content

refactor keeper auth configuration#2052

Merged
moolen merged 1 commit intoexternal-secrets:mainfrom
enreach-labs:refactor-keeper-auth-configuration
Feb 27, 2023
Merged

refactor keeper auth configuration#2052
moolen merged 1 commit intoexternal-secrets:mainfrom
enreach-labs:refactor-keeper-auth-configuration

Conversation

@pepordev
Copy link
Copy Markdown
Contributor

@pepordev pepordev commented Feb 24, 2023

Problem Statement

Refactor keeperSecurity integration to simplify authentication

Related Issue

Fixes #2048

Proposed Changes

How do you like to solve the issue and why?

Checklist

  • I have read the contribution guidelines
  • All commits are signed with git commit --signoff
  • My changes have reasonable test coverage
  • All tests pass with make test
  • I ensured my PR is ready for review with make reviewable

Signed-off-by: Pedro Parra Ortega <parraortega.pedro@gmail.com>
@pepordev pepordev requested a review from a team as a code owner February 24, 2023 12:22
@pepordev pepordev requested review from moolen and removed request for a team February 24, 2023 12:22
Copy link
Copy Markdown
Contributor

@paul-the-alien paul-the-alien bot left a comment

Choose a reason for hiding this comment

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

Greetings!
Thank you for contributing to this project!
If this is your first time contributing, please make
sure to read the Developer and Contributing Process guides.
Please also mind and follow our Code of Conduct.

Useful commands:

  • make fmt: Formats the code
  • make check-diff: Ensures the branch is clean
  • make reviewable: Ensures a PR is ready for review

@sonarqubecloud
Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Copy Markdown
Member

@moolen moolen left a comment

Choose a reason for hiding this comment

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

That makes sense 👍 ! I'm glad we haven't released this yet

@moolen moolen merged commit 2766c6d into external-secrets:main Feb 27, 2023
@maksimu
Copy link
Copy Markdown

maksimu commented Feb 28, 2023

@ppodevlab for some reason the docs are broken https://external-secrets.io/main/provider/keeper-security/

@moolen
Copy link
Copy Markdown
Member

moolen commented Feb 28, 2023

I'll take care of that, this seems to be an issue with mkdocs

@maksimu
Copy link
Copy Markdown

maksimu commented Feb 28, 2023

@moolen it works now! thank you for the fix.
@ppodevlab Great work!
One more minor comment: I see that you are still asking for the hostname (see HERE), I would simpliefy the configuration even further and remove it, as the hostname is already inside of the JSON/Base64 config. Our SDKs will deduct which host to use using the first letters in the one-time token (ex: EU:abc123ABC... or JP:abc123ABC...)

Also, just FYI, regarding generating Base64 Config, we have it in the part where you are adding new Device screen (unfortunately it is not yet as a part of the Add New Application screen yet)

image

@pepordev pepordev deleted the refactor-keeper-auth-configuration branch March 1, 2023 06:39
@pepordev
Copy link
Copy Markdown
Contributor Author

pepordev commented Mar 1, 2023

@moolen it works now! thank you for the fix. @ppodevlab Great work! One more minor comment: I see that you are still asking for the hostname (see HERE), I would simpliefy the configuration even further and remove it, as the hostname is already inside of the JSON/Base64 config. Our SDKs will deduct which host to use using the first letters in the one-time token (ex: EU:abc123ABC... or JP:abc123ABC...)

Also, just FYI, regarding generating Base64 Config, we have it in the part where you are adding new Device screen (unfortunately it is not yet as a part of the Add New Application screen yet)

image

you are totally right. As this hasn't been released yet i will update the configuration.

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.

Simplify Initialization of Keeper Secrets Manager Integration

3 participants