Skip to content

Conversation

@daeMOn63
Copy link

No description provided.

@daeMOn63 daeMOn63 changed the base branch from ctap2 to master September 28, 2020 12:40
pin input now use golang.org/x/crypto/ssh/terminal to not be
echoed on stdout anymore. The library is expected to work on *any* go
supported OS terminals, but only tested on linux.
Copy link
Contributor

@titanous titanous left a comment

Choose a reason for hiding this comment

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

Posting my incomplete review from a few months ago. Will do another pass later.

BaseIV []byte `cbor:"5,keyasint,omitempty"`
}

func (k *COSEKey) CBOREncode() ([]byte, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func (k *COSEKey) CBOREncode() ([]byte, error) {
func (k *COSEKey) MarshalCBOR() ([]byte, error) {

Copy link
Author

Choose a reason for hiding this comment

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

return getpasswd(h.Stdin)
}

func (h *InteractiveHandler) SetPIN(token *ctap2token.Token) ([]byte, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like most of this handler should be abstracted out into a helper that other handlers can use? I'd expect it to only do the collection from stdin and then call a shared function that does all the work.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, done.

}

// MakeCredentialResponse
// TODO: structure may be different with different kind of attestations.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this TODO still pending?

Copy link
Author

Choose a reason for hiding this comment

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

Hmm no. The MakeCredentialResponse structure is fixed and only the content of AttSmt may vary, but since it's a map[string]interface{} we should be good to store anything. Thanks, removed.

HyperSecu Mini tokens are reporting errors when calling GetAssertion.
It seems these tokens don't support the transports field being set on
the CredentialDescriptor. Luckily it's optionnal so we can safely remove
it.
HyperSecu mini tokens returns wrong error on timeout
that we can catch in order to avoid asking for user pin
when it's not set
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.

4 participants