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

Add explicit support for identities stored on hardware keys … #78

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yoavamit
Copy link

…(like Yubikey).

Since Apple no longer support enumerating certificates stored on hardware keys in the Keychain Access application,
this PR explicitly tries enumerate certificates stored in the "signature" slot for hardware keys that support PIV applets.

Implementation details:

  • The hardware key PIN is prompted for at the beginning to make sure we don't interfere with the output git expects while signing
  • To make this as easy as possible, this PR adds a new struct called PivIdentity which implements certstore.Identity interface
  • The PivIdentity struct has an open handle to a *piv.Yubikey and needs to be closed properly when done using it

@yoavamit yoavamit force-pushed the yoav.add-piv-applet-support branch 2 times, most recently from 057d067 to 9d625ce Compare May 17, 2021 20:34
@vcsjones vcsjones self-requested a review May 28, 2021 14:20
@yoavamit yoavamit force-pushed the yoav.add-piv-applet-support branch 2 times, most recently from aebbe78 to 287c142 Compare June 3, 2021 14:16
Copy link
Member

@vcsjones vcsjones left a comment

Choose a reason for hiding this comment

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

I like the idea of this to better support PIV devices. I have some questions about the implementation and how this might/could work on other platforms. Thanks!

piv_identity.go Outdated
}
defer tty.Close()

_, err = fmt.Fprintf(tty, "enter PIN for hardware key \"%v\" (press enetr for the default PIN):\n", ident.card)
Copy link
Member

Choose a reason for hiding this comment

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

Small typo.

Suggested change
_, err = fmt.Fprintf(tty, "enter PIN for hardware key \"%v\" (press enetr for the default PIN):\n", ident.card)
_, err = fmt.Fprintf(tty, "enter PIN for hardware key \"%v\" (press enter for the default PIN):\n", ident.card)

piv_identity.go Outdated
}

func (ident *PivIdentity) promptHardwareKeyPin() (string, error) {
tty, err := os.OpenFile("/dev/tty", os.O_RDWR, fs.ModeCharDevice)
Copy link
Member

Choose a reason for hiding this comment

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

What is the behavior of this if a TTY is not available?

For example: let's suppose someone is committing with a GUI like GitHub Desktop. There won't be a TTY where someone can type the device PIN. Will this error or will it hang waiting for someone to type a pin?

I'm less worried about supporting the GUI with a modal, I just want to understand the behavior.

Further, I suppose this will not work on Windows, correct? Windows' will import certificates in to its certificate store with the Certificate Propagation Service (or middleware). Should this functionality be omitted for Windows?

Copy link
Author

Choose a reason for hiding this comment

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

I don't think it's going to work very well on other platforms or when using a GUI git client.
I realized I might've submitted this PR a little too early, since I just also found out that I'm not handling the certificate chain the right way either.

For context though, it seems like the PIN prompt is also a big issue with gpgsm.
Looking at the source code there, there's a whole library dedicated to handle the PIN prompt.

What I did here is extract the most basic and simplified scenario from that library so it'll at least be functional when using a shell.

Copy link
Author

Choose a reason for hiding this comment

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

@vcsjones I did a little more research on how this procedure usually works, and decided to go with a different route to get the PIN when using a Yubikey.

If a PIN is required now, smimesign will try to get it via the pinentry program.
pinentry is part of GnuPG tools and is considered to be a more secure way of getting such information from the user.

It also has a lot of different implementations for different platforms, so the user can choose which program to use if they want to.

I've tested this PR using both the terminal and Github Desktop on a MacBook.
I didn't get a chance to test it on other platforms yet.

piv_identity.go Outdated
hardwareKeyPin = string(pin)
}

_, _ = fmt.Fprintf(tty, "Touch %v now unlock\n", ident.card)
Copy link
Member

Choose a reason for hiding this comment

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

This seems specific to Security Key devices with a PIV applet. Does this text need to be changed to consider other smart-card PIV devices (like an actual SmartCard) that do not a user-presence for touch?

Or is this functionality for YubiKeys only?

Copy link
Author

Choose a reason for hiding this comment

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

The library that I used here is intended for Yubikeys.

@yoavamit yoavamit force-pushed the yoav.add-piv-applet-support branch from 0843302 to 06f426b Compare June 10, 2021 16:15
…ikey)

Since Apple no longer support enumerating certificates stored on hardware keys in the Keychain Access application,
this PR explicitly tries enumerate certificates stored in the "signature" slot for hardware keys that support PIV applets.

Implementation details:
- The hardware key PIN is prompted for at the beginning to make sure we don't interfere with the output git expects while signing
- To make this as easy as possible, this PR adds a new struct called `PivIdentity` which implements `certstore.Identity` interface
- The `PivIdentity` struct has an open handle to a `*piv.Yubikey` and needs to be closed properly when done using it
@yoavamit yoavamit force-pushed the yoav.add-piv-applet-support branch from 06f426b to 1502d64 Compare June 11, 2021 17:10
@unix-system
Copy link

Is there any chance this could be merged? I'm happy to help if it needs a review or work.

@vcsjones On another note, is this tool still maintained actively and accepting PRs? The latest commit to the main branch was nearly a year ago 😢

This tool is possibly one of the most useful I've found for what I'm trying to achieve, and other than using gpgsm with lots of plugins and configurations (which make it nearly unusable for the end user), it's essentially the only solution out there. Possibly one of the most useful open source products I've found and so would be happy to do anything I can to help!

@yoavamit
Copy link
Author

Is there any chance this could be merged? I'm happy to help if it needs a review or work.

@vcsjones On another note, is this tool still maintained actively and accepting PRs? The latest commit to the main branch was nearly a year ago 😢

This tool is possibly one of the most useful I've found for what I'm trying to achieve, and other than using gpgsm with lots of plugins and configurations (which make it nearly unusable for the end user), it's essentially the only solution out there. Possibly one of the most useful open source products I've found and so would be happy to do anything I can to help!

I've actually moved on to use a different tool called pivit

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