Implement Read/Write CHUID #67
Conversation
piv/piv.go
Outdated
| // Raw contains the whole object | ||
| // GUID contains the Card Universally Unique Identifier. | ||
| type CardID struct { | ||
| Raw []byte |
There was a problem hiding this comment.
Make raw unexported and in a future PR add whatever other field you want to be able to inspect. FASC-N, Exp. Date, etc.
Though if you want to use the signature in the API in a follow up PR, I think I'd prefer something like:
func (c *CardID) Verify(pub crypto.PublicKey) error
func (c *CardID) Sign(priv crypto.Signer) error
piv/piv.go
Outdated
| return id, fmt.Errorf("unmarshaling response: %v", err) | ||
| } | ||
| id.Raw = obj | ||
| if obj[27] == 0x34 { |
There was a problem hiding this comment.
Before doing any slice indexes you need to verify the length of the returned object.
if len(obj) < n {
// return error
}
uuid := obj[n:]
piv/piv.go
Outdated
| } | ||
| id.Raw = obj | ||
| if obj[27] == 0x34 { | ||
| endPos := uuidOffset + int(obj[28]) |
There was a problem hiding this comment.
UUID length must be 16 bytes? https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-73-4.pdf#page=26
Return an error if it's not?
piv/piv.go
Outdated
| // - 0x35: Exp. Date (hard-coded) | ||
| // - 0x3e: Signature (hard-coded, empty) | ||
| // - 0xfe: Error Detection Code (hard-coded) | ||
| func (yk *YubiKey) SetCardID(GUID [16]byte, key [24]byte) (CardID, error) { |
There was a problem hiding this comment.
Can this be:
func(yk *YubiKey) SetCardID(key [24], cardID *CardID) error
- All other methods in this package have the management key/PIN/PUK as their first argument
- If we ever wanted other methods to be settable (e.g. signature) then we don't need to add new APIs
- We don't return a CardID, a successful SetCardID should imply that the state on the card is equivalent to the passed CardID
piv/piv.go
Outdated
| } | ||
|
|
||
| // CardID returns the card CHUID with the GUID extracted | ||
| func (yk *YubiKey) CardID() (CardID, error) { |
There was a problem hiding this comment.
For the doc comment: can you call this method before calling SetCardID?
nit: for consistency with other APIs in this package, return a pointer
func (yk *YubiKey) CardID() (*CardID, error)
piv/piv.go
Outdated
| } | ||
|
|
||
| // SetCardID initialize the CHUID card object using a predefined template defined as | ||
| // * Defined fields: |
There was a problem hiding this comment.
The Defined fields is really helpful for review 😃 but should be in the doc comment. Doc comment should only document what's exposed in the API
| * - 0xfe: Error Detection Code (hard-coded) | ||
| */ | ||
|
|
||
| var chuidTemplate = []byte{ |
There was a problem hiding this comment.
Where does this come from? Would default field values be better?
There was a problem hiding this comment.
This is coming straight from yubico tools. they generated that - for now, it would suggest to do the same, and later add the fields in the CardID struct and add the encoding.
piv/piv.go
Outdated
|
|
||
| func ykSetCardID(tx *scTx, key [24]byte, guid [16]byte) (id CardID, err error) { | ||
|
|
||
| id.Raw = make([]byte, len(chuidTemplate)) |
There was a problem hiding this comment.
Just build the fields yourself with default values instead of using a template?
data := marshalASN1(/* tag */, defaultFASCN)
data = append(data, marshalASN1(/* tag */, guid[:]))
data = append(data, marshalASN1(/* tag */, defaultExp))
data = append(data, marshalASN1(/* tag */, defaultSig))
data = append(data, marshalASN1(/* tag */, defaultErrorCorrectionCode))
That way if you ever want to make something like the signature writable, it's clear where to modify the value.
There was a problem hiding this comment.
can we do that in a future PR as it would be better to pass a full cardID and not have any default ?
| * - 0x30: FASC-N (hard-coded) | ||
| * - 0x34: Card UUID / GUID (settable) | ||
| * - 0x35: Exp. Date (hard-coded) | ||
| * - 0x3e: Signature (hard-coded, empty) |
There was a problem hiding this comment.
Are these actually not settable or is that just the API that yubico's tools expose?
There was a problem hiding this comment.
They can be set - it's hardcoded in the yubico implementation, i was thinking of eventually add these and implement the BCD encoding too in a future PR.
piv/piv.go
Outdated
| return nil, fmt.Errorf("unexpected uuid length value: %d", obj[28]) | ||
| } | ||
| endPos := uuidOffset + int(obj[28]) | ||
| copy(id.GUID[:], obj[uuidOffset:endPos]) |
There was a problem hiding this comment.
endPos can still be longer than the length of obj if the card returns bad data, resulting in a crash.
Can you do the same logic for decoding the metadata instead? Iterate through the raw ASN.1 objects until you find the UUID value?
Line 775 in e6548dd
|
Still plan to take this, but took some extra time off for the long weekend. Will look again when I'm back :) |
|
Any way to get this reviewed/merged ? Thanks |
Ready to merge