Improve error types to allow wrapping

[dnesting]

I got stymied not having an idiomatic way of seeing whether a smartcard object was not found or not, and I saw the TODO in the code to list error cases, so I thought I'd offer this improvement for comment.

• Promotes apduError to a public ApduError type
• Adds error messages from the latest PIV spec
• Uses error Unwrap() to return a few more accessible/idiomatic Go errors to enable some errors.Is and errors.As uses, such as ErrNotFound, ErrBlocked
• Promotes errWrongPin to VerifyErr type
• Changes all %v verbs to %w when wrapping errors (new in 1.13, which is what go.mod says we are)
• The above changes made ykTransmit unnecessary, so removed

I can now do things like:

// Don't overwrite a slot
_, err = yk.Certificate(slot)
if err == nil {
  return errSlotOccupied
} else if !errors.Is(err, piv.ErrNotFound) {
  return err
}

Fixes #13

[ericchiang]

I'm fine with ErrNotFound and improving the error message. Please add a test for that one :)

Less convinced that we need to expose so much API surface to deal with errors. Maybe ErrWrongPIN is worth exposing, but what practical program is going to do anything except return that error to the user? Definitely think that we shouldn't expose apduError. It's way too low level, and I'd rather add specific error conditions to the API, like an object not being found.

[ericchiang]

Think this block shows why we should just have one error type. Maybe s/VerifyErr/AuthErr/g (Verify sounds like it's an issue with a signature).

var e *AuthErr
if !errors.As(err, &e) {
	return fmt.Errorf("blocking pin: %v", err)
}
if e.Retries == 0 {
	break
}

[dnesting]

So I was operating under the belief that you would want to distinguish between the retries==0 case and the blocked case, but I see from experimentation that it goes from retries=2 retries=1 to blocked, so representing blocked as AuthErr(retries=0) seems safe? I'll make that change.

[dnesting]

OK, that makes sense. I'll pull this back to expose less, which means the %w changes add less value so I'll drop those as well.

The ykTransmit drop was intentional, though, as this function was essentially a no-op after the error handling got moved out of it. This will be clearer with the reversions suggested.

[dnesting]

OK I believe I've made all of the changes you suggested and added tests. PTAL.

[ericchiang]

lgtm, one nit

Also can you please squash your commits? I'll merge after

[ericchiang]

nit: can this stay as a struct? don't know if there might be additional data we might want to add, but just to play it safe.

type AuthErr struct {
    Retries int
}

[dnesting]

Sure.

[dnesting]

Changes made and squashed.

I also made one additional small change to the error message text returned by apduErr.Error() for the 'blocked' case, to allow someone to distinguish between "verification failed" and "method blocked" there since AuthErr has the same value for each. And added a test for that.

[ericchiang]

Thanks for the couple iterations :) If you hit any other use cases where it'd be good to expose another error type, please let me know. Will tag a release later this evening