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 slot name to Attestation struct #89

Merged
merged 1 commit into from
Jul 9, 2021
Merged

Add slot name to Attestation struct #89

merged 1 commit into from
Jul 9, 2021

Conversation

jalseth
Copy link
Contributor

@jalseth jalseth commented Jul 5, 2021

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

Copy link
Collaborator

@ericchiang ericchiang left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. Holidays :)

piv/key.go Outdated Show resolved Hide resolved
piv/key.go Outdated Show resolved Hide resolved
@jalseth
Copy link
Contributor Author

jalseth commented Jul 6, 2021

@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.

piv/key.go Outdated Show resolved Hide resolved
piv/key.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@ericchiang ericchiang left a comment

Choose a reason for hiding this comment

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

lgtm, just a couple of nits

piv/key.go Outdated Show resolved Hide resolved
piv/key_test.go Outdated Show resolved Hide resolved
@ericchiang
Copy link
Collaborator

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.

piv/key.go Outdated Show resolved Hide resolved
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
Copy link
Contributor Author

jalseth commented Jul 9, 2021

@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 ericchiang merged commit 23adea5 into go-piv:master Jul 9, 2021
@ericchiang
Copy link
Collaborator

Thanks for you work here!

@jalseth jalseth deleted the add-slot-to-verified-attestation branch July 9, 2021 03:00
@jalseth
Copy link
Contributor Author

jalseth commented Jul 9, 2021

🎉

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

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.

None yet

2 participants