Initial yubikey backend keytype support #418
Conversation
|
Please stop making new PRs :) Rebase and force-push the branch in-place is much easier instead of leaving 3 PRs around in the project. |
|
Sure, I'll do that going forward, I didn't know that worked in a github PR my bad! |
|
Would it be helpful if rebased and force pushed this commit PR over #413? I can then use this one for the prompt support PR |
Nah, no we can work in this branch :) |
| PublicKey string `json:"PublicKey"` | ||
| } | ||
|
|
||
| var YK *piv.YubiKey |
There was a problem hiding this comment.
Any reason we can't throw the piv connetion into the Yubikey struct?
There was a problem hiding this comment.
I tried to do that originally but I ended up needing to open up a piv connection and track piv state in YubikeyFromBytes during the signing workflow and it became more complicated. I ended up going with the most simple approach to start
There was a problem hiding this comment.
But can't we just have a pointer in the struct and use connectToYubikey() as a struct method when the actual card is needed?
Everything seems fairly self-contained at the moment?
There was a problem hiding this comment.
That approach works for creating keys, but the tricky part is in the Signer interface. the piv-go library only lets you have one handle to the yubikey open at once (https://github.com/go-piv/piv-go/blob/0383b0aa884b2b642e9e3446ea01ba22ccadc83a/v2/piv/piv.go#L107). When you defer yk-piv.Close() after creating the handle in Signer() the signing operation happens after the function is returned and by then the handle is closed. If you leave the handle open you can't open a new handle to the yubikey for future operations.
I can make it work but I would likely need to change the Signer interface to return a function that gets called after signing operations to appropriately Close the handle. What do you think about that approach?
|
Updated with the requested changes, I also verified that using sbctl to create a key with the yubikey type and signing files is working (for me at least) with the default pin |
| PublicKey string `json:"PublicKey"` | ||
| } | ||
|
|
||
| var YK *piv.YubiKey |
There was a problem hiding this comment.
But can't we just have a pointer in the struct and use connectToYubikey() as a struct method when the actual card is needed?
Everything seems fairly self-contained at the moment?
| yubiData := YubikeyData{ | ||
| Info: *f.pubKeyInfo, | ||
| Slot: "signature", | ||
| PublicKey: base64.StdEncoding.EncodeToString(x509.MarshalPKCS1PublicKey(f.pubKeyInfo.PublicKey.(*rsa.PublicKey))), |
There was a problem hiding this comment.
We same f.pubKeyInfo here, why do we have YubiConfig.PubKeyInfo as well stored as part of the state?
Is this to prevent us from reading the information from the yubikey multiple times?
There was a problem hiding this comment.
It's also stored in the config to make sure when you want to overwrite or create a new one, it only creates a new one on signature slot in the yubikey once
Same PR for initial yubikey piv support with the intermediate commits removed.
I also removed:
Hopefully this helps for reviewing / testing. I think I also addressed all your comments from the previous review. Thank you for your help!
I do think the prompting for the PIN is important to avoid needing to store the pin anywhere, so once we get this PR done I can rework and submit that one separately