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

Use passphrase terminology, not PIN #57

Merged
merged 4 commits into from
Aug 16, 2024
Merged

Conversation

dcousens
Copy link
Contributor

@dcousens dcousens commented Jul 14, 2024

As part of f8a5360, you used the phrasing passphrase, not PIN; and the usage seems inconsistent throughout.

This pull request replaces any usage of pin with passphrase (or pass internally), such that the usage and terminology (and stdout output) is somewhat consistent with ssh-keygen.

I think pointing out that TPM 2.0 has well defined anti-hammering protection is informative as to the type of passphrase you could use compared to hinting specifically that you have PIN support; which nearly sounds like a limitation to me than a feature.

However I am not too opinionated on keeping ce6edea in this pull request if you feel otherwise.

agent/agent_test.go Outdated Show resolved Hide resolved
cmd/ssh-tpm-keygen/main.go Outdated Show resolved Hide resolved
@Foxboron
Copy link
Owner

The issue is that passphrase implies that you write a longer passphrase. The point of using "PIN" to tell people that with DA procetion you can have a 4 digit numerical pin as protection for the key. Writing "TPM Passphrase" does not convey this.

The reason why it says so in the askpass prompt is because there are askpass programs that cache the "passphrase" based off on the "askpass" prompt. Thus you end up with inconsistentcies like this.

So I'm not really convinced I want all of these changes.

@dcousens
Copy link
Contributor Author

dcousens commented Jul 16, 2024

That's fair, and I'm thinking about how maybe we can balance the best of both.
I'll mark this as draft for now, unless you only wanted me to drop ce6edea?

@dcousens dcousens marked this pull request as draft July 16, 2024 22:46
@Foxboron
Copy link
Owner

I'll need to think a bit. Internally in the codebase I think it's better to use userauth instead of pin or pass to better convey what it actually is in the context of the TPM.

@Foxboron
Copy link
Owner

Okay, I've decided.

Drop the change from "pin" to "pass" in the codebase as I plan on changing the references to "userauth" which is the correct word in the context of TPMs.

I think we should drop the README.md change and I'll ponder on a better sentence. The prompt changes from "pin" to "passphrase" to keep it consistent with openssh is fine.

Does all of that sounds fine?

@dcousens dcousens marked this pull request as ready for review August 16, 2024 05:53
@dcousens
Copy link
Contributor Author

@Foxboron I think I have made the changes requested, should be good for review

@dcousens
Copy link
Contributor Author

I additionally added d45fd23, which seems to be different - I can drop that if needed

@Foxboron Foxboron merged commit e2c978d into Foxboron:master Aug 16, 2024
5 checks passed
@dcousens dcousens deleted the pass-not-pin branch August 17, 2024 05:01
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.

2 participants