go-piv / piv-go

Keys and certificates for YubiKeys, written in Go
Apache License 2.0
366 stars 65 forks source link

Add slot name to Attestation struct #89

Closed jalseth closed 3 years ago

jalseth commented 3 years ago

In some use cases, knowing what slot the attested key resides in can be useful for determining whether to issue a certificate for the key.

Signed-off-by: James Alseth james@yubico.com

jalseth commented 3 years ago

@ericchiang No need to apologize, a review in under a day is very quick! I believe I've addressed your feedback, and removed the split altogether.

ericchiang commented 3 years ago

Finally tested this on a machine. It passes, and I verified it works with the following change as well:

diff --git a/piv/key_test.go b/piv/key_test.go
index 429cb7e..f81c2ea 100644
--- a/piv/key_test.go
+++ b/piv/key_test.go
@@ -433,6 +433,9 @@ func TestYubiKeyAttestation(t *testing.T) {
        if a.Version != yk.Version() {
                t.Errorf("attestation version got=%#v, wanted=%#v", a.Version, yk.Version())
        }
+       if a.Slot != SlotAuthentication {
+               t.Errorf("attested slot got=%v, wanted=%v", a.Slot, SlotAuthentication)
+       }
 }

Can you please squash your commits? Feel free to add that test change, or I can too.

jalseth commented 3 years ago

@ericchiang I didn't run into the issue with using String() on the pointer in my test, but it does make sense to not use a pointer reference when the item in the struct is not a pointer.

I've squashed everything so I think we're set. Thanks for working with me on this!

ericchiang commented 3 years ago

Thanks for you work here!

jalseth commented 3 years ago

:tada:

@ericchiang When could this be expected in a release? No huge rush, but it would be nice to not have to use the replace directive in my go.mod file.

ericchiang commented 3 years ago

Tagged https://github.com/go-piv/piv-go/releases/tag/v1.8.0