-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
cmd: Add client secret encryption option #1322
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good! It would be great if you could add an example to the help command, somewhere around here.
Also, it would probably make sense to have this feature in clients import
as well. Maybe move this stuff to a helper function (which would also allow testing this functionality) and use it for both import and create!
Thanks for reviewing! |
cmd/cli/handler_client.go
Outdated
@@ -56,6 +56,11 @@ func (h *ClientHandler) ImportClients(cmd *cobra.Command, args []string) { | |||
cmdx.MinArgs(cmd, args, 1) | |||
m := h.newClientManager(cmd) | |||
|
|||
ek, encryptSecret, err := newEncryptionKey(cmd, nil) | |||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cmdx.Must
will automatically skip if err == nil
, so you can leave out the if
check!
cmd/cli/handler_client.go
Outdated
@@ -92,6 +109,11 @@ func (h *ClientHandler) CreateClient(cmd *cobra.Command, args []string) { | |||
fmt.Println("You should not provide secrets using command line flags, the secret might leak to bash history and similar systems") | |||
} | |||
|
|||
ek, encryptSecret, err := newEncryptionKey(cmd, nil) | |||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could add a test with cmd line flags here: https://github.com/ory/hydra/blob/master/cmd/root_test.go#L116 |
Signed-off-by: Shota Sawada <xiootas@gmail.com>
Signed-off-by: Shota Sawada <xiootas@gmail.com>
Signed-off-by: Shota Sawada <xiootas@gmail.com>
Signed-off-by: Shota Sawada <xiootas@gmail.com>
Thanks! I added test PGP keys and test case :) |
Awesome! Thanks :) |
Related issue
#1317
Proposed changes
Add client secret encryption option.
It behaves following
hydra clients create --pgp-key=<base64 encoded PGP encryption key> ...
hydra clients create --pgp-key-url=<PGP encryption key URL> ...
hydra clients create --keybase=<Keybase username> ...
Then stdout will be like following
To decrypt this, private key holder executes base64 decode and PGP decrypt.
Following is an example for keybase.
When an error occurred, client create request will be canceled and stdout will be like following
Full Example
Create Client With Client Secret Encryption
Import Client
Decrypt Client Secret
Error Pattern
Checklist
vulnerability, I confirm that I got green light (please contact hi@ory.sh) from the maintainers to push the changes.
by signing my commit(s). You can amend your signature to the most recent commit by using
git commit --amend -s
. If youamend the commit, you might need to force push using
git push --force HEAD:<branch>
. Please be very careful when usingforce push.
Further comments
I put implementation for encryption my repository because I'm not sure where to put.
Let's talk about it. And when it will be clear where to put it, I can re-implement hydra or ory's library.
https://github.com/sawadashota/encrypta