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

Implement Read/Write CHUID #67

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Implement Read/Write CHUID #67

wants to merge 5 commits into from

Conversation

jc-m
Copy link

@jc-m jc-m commented May 17, 2020

Ready to merge

@jc-m jc-m changed the title Initial CHUID reading Implement Read/Write CHUID May 18, 2020
piv/piv.go Outdated
// Raw contains the whole object
// GUID contains the Card Universally Unique Identifier.
type CardID struct {
Raw []byte
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

agreed

piv/piv.go Outdated
return id, fmt.Errorf("unmarshaling response: %v", err)
}
id.Raw = obj
if obj[27] == 0x34 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Before doing any slice indexes you need to verify the length of the returned object.

if len(obj) < n {
    // return error
}
uuid := obj[n:]

Copy link
Author

Choose a reason for hiding this comment

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

agreed

piv/piv.go Outdated
}
id.Raw = obj
if obj[27] == 0x34 {
endPos := uuidOffset + int(obj[28])
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

agreed

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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

agreed

piv/piv.go Outdated
}

// CardID returns the card CHUID with the GUID extracted
func (yk *YubiKey) CardID() (CardID, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

Copy link
Author

Choose a reason for hiding this comment

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

agreed

piv/piv.go Outdated
}

// SetCardID initialize the CHUID card object using a predefined template defined as
// * Defined fields:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

agreed

* - 0xfe: Error Detection Code (hard-coded)
*/

var chuidTemplate = []byte{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where does this come from? Would default field values be better?

Copy link
Author

Choose a reason for hiding this comment

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

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))
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these actually not settable or is that just the API that yubico's tools expose?

Copy link
Author

Choose a reason for hiding this comment

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

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])
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

if !bytes.HasPrefix(v.FullBytes, []byte{0x89}) {

@ericchiang
Copy link
Collaborator

Still plan to take this, but took some extra time off for the long weekend. Will look again when I'm back :)

@jc-m
Copy link
Author

jc-m commented Sep 10, 2020

Any way to get this reviewed/merged ? Thanks

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