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: adding support for exposing secrets to subprocesses #68

Merged
merged 5 commits into from
Feb 23, 2025

Conversation

Nelwhix
Copy link
Contributor

@Nelwhix Nelwhix commented Jan 28, 2025

This PR adds the feature discussed in #67

@CLAassistant
Copy link

CLAassistant commented Jan 28, 2025

CLA assistant check
All committers have signed the CLA.

@speatzle
Copy link
Collaborator

Hi, Sorry for the long delay. I was sick the last couple of weeks an am now finally catching up to everything.

I had a brief look at your PR, look pretty good without actually testing it.

Here are my thoughts:

We need a section in the README, the Help description is pretty good, i think you can mostly just copy paste that.

Could you also add a Debug log with which Enviroment variables where filled with which Resource?

We also need a way to specify which part of the Resource we should put into the Environment. With New Resource Types and the Ability to add Custom Resource Types on the Horizon i feel its important to at least have the syntax for that fleshed out. We can Merge this PR without this implemented.

As an Example: A User might want to put the Username in the Environment or with a SSH Resource Type the SSH Private Key.

Since a Resource has essentially 2 Parts, the Metadata and the Secret data so also need to have a way to specify from which of these 2 the Data is supposed to be from, but this might be skipable if the Field is unique.

We could use a path style like passbolt://<PASSBOLT_RESOURCE_ID_HERE>/meta/username, passbolt://<PASSBOLT_RESOURCE_ID_HERE>/secret/ssh_key or without the implicit part passbolt://<PASSBOLT_RESOURCE_ID_HERE>/ssh_key

I have also though about using dots as separators but that doesn't really work the URI style we have here.

Implementing the Field selection can't be done in a good way Right now with how Resources are stored in go-passbolt. But this will have to be properly fixed with breaking changes in go-passbolt for v5 anyway, ill write more on that here passbolt/go-passbolt#23

Any thoughts on the Field selection?

@Nelwhix
Copy link
Contributor Author

Nelwhix commented Feb 18, 2025

We could use a path style like passbolt://<PASSBOLT_RESOURCE_ID_HERE>/meta/username, passbolt://<PASSBOLT_RESOURCE_ID_HERE>/secret/ssh_key or without the implicit part passbolt://<PASSBOLT_RESOURCE_ID_HERE>/ssh_key

Hi @speatzle, hope you are feeling better. I think this field selection style is good. so once we refactor go-passbolt, we can modify this exec command as well. Btw I have modifed the readme and added the debug logs

@speatzle
Copy link
Collaborator

I ran some tests and it all works as i expect it to.

One small thing, can you drop "error" from the Error messages and Correctly Capitalize the rest of the Messages?

I think i can merge as soon as that's done.

@Nelwhix
Copy link
Contributor Author

Nelwhix commented Feb 19, 2025

dropped "error"...but capitalizing error messages is against google's go style guide https://google.github.io/styleguide/go/decisions.html#error-strings

@speatzle
Copy link
Collaborator

I know that it is against Googles style guide, but all errors in go-passbolt-cli and go-passbolt where Capitalised a long time ago, before the Project was on github. At this point we can't change any of them without risk of breaking lots of things that depend on both of them.

So i would like to just Keep it consistent with the Rest of the Code.

https://github.com/search?q=repo%3Apassbolt%2Fgo-passbolt-cli%20fmt.error&type=code

https://github.com/search?q=repo%3Apassbolt%2Fgo-passbolt%20fmt.error&type=code

@Nelwhix
Copy link
Contributor Author

Nelwhix commented Feb 23, 2025

Okay

@Nelwhix
Copy link
Contributor Author

Nelwhix commented Feb 23, 2025

Updated

@speatzle speatzle merged commit 1183813 into passbolt:main Feb 23, 2025
1 check passed
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.

3 participants