-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Display secret to user in inspect #19011
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4c62cc5
to
5949e58
Compare
@ashley-cui PTAL |
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.
Again, this is nitpicky on naming, but I feel like there could be a better name - perhaps --show-data
or --display-data
? I don't think --display
is descriptive enough as a flag for something as revealing as dumping the actual contents of the secret, and doesn't make sense for API calls
Otherwise, implementation and code LGTM
Not a big fan of options with "-" embeded. How about --expose
|
|
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.
Code LGTM but I have absolutely no feeling for the naming.
I think that @TomSweeneyRedHat and @edsantiago have good nose for such things.
I've been lurking here, mulling it over in the background, and my medium preference is for Reason: that is (to me) the scariest-sounding name. It triggers a strong visceral response, my fingers don't even want to type it. Before I type such a command I would spend time reading the man page and help output. Or I might decide not to do it at all. And I think that, for a risky option like this one, that is a desirable reaction. |
I would prefer something that I can understand without reading the man page, i.e. |
We seem to be painting a shed.... Since the secret is actually just base64 |
That's a detail of implementation for the built-in file driver. In the future when we have other and possibly remote secret drivers, that would not be so logical. |
I agree. A I think it's good practice to highlight the "don't try this at home" aspect of the flag. It's not a secret anymore once it's out :^) |
But doesn't |
It is pretty complicated to display the secret on the host, but is not really secured. This patch makes it easier to examine the secret. Partial fix for containers#18667 Signed-off-by: Daniel J Walsh <[email protected]>
Went with --showsecret |
LGTM |
LGTM, and I like showsecret too. |
Another reminder to stick to the 2 LGTM rule (and not merging our own PRs). Rushing new features in is risky, especially so close to a new release. |
It is pretty complicated to display the secret on the host, but is not really secured. This patch makes it easier to examine the secret.
Partial fix for #18667
Does this PR introduce a user-facing change?