-
Notifications
You must be signed in to change notification settings - Fork 14
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 VALIDATE calls #13
base: master
Are you sure you want to change the base?
Changes from 4 commits
13ca424
4e940af
3e1f718
0bb0699
b083034
fd081cb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,23 +3,33 @@ package ykoath | |
import ( | ||
"encoding/binary" | ||
"fmt" | ||
"math" | ||
"strings" | ||
) | ||
|
||
const ( | ||
errNoValuesFound = "no values found in response (% x)" | ||
errUnknownName = "no such name configued (%s)" | ||
errNoValuesFound = "no values found in response (% x)" | ||
errUnknownName = "no such name configued (%s)" | ||
errMultipleMatches = "multiple matches found (%s)" | ||
touchRequired = "touch-required" | ||
touchRequired = "touch-required" | ||
) | ||
|
||
// NoTouchCallback performas a noop for users that have no touch support | ||
func NoTouchCallback(_ string) error { | ||
return nil | ||
} | ||
|
||
// ErrorTouchCallback raises an error for users that have no touch support | ||
func ErrorTouchCallback(_ string) error { | ||
return fmt.Errorf("requires touch but no callback specified") | ||
} | ||
|
||
// Calculate is a high-level function that first identifies all TOTP credentials | ||
// that are configured and returns the matching one (if no touch is required) or | ||
// fires the callback and then fetches the name again while blocking during | ||
// the device awaiting touch | ||
func (o *OATH) Calculate(name string, touchRequiredCallback func(string) error) (string, error) { | ||
|
||
res, err := o.calculateAll() | ||
res, err := o.CalculateAll() | ||
|
||
if err != nil { | ||
return "", nil | ||
|
@@ -50,17 +60,17 @@ func (o *OATH) Calculate(name string, touchRequiredCallback func(string) error) | |
return "", err | ||
} | ||
|
||
return o.calculate(key) | ||
return o.CalculateOne(key) | ||
|
||
} | ||
|
||
return code, nil | ||
|
||
} | ||
|
||
// calculate implements the "CALCULATE" instruction to fetch a single | ||
// CalculateOne implements the "CALCULATE" instruction to fetch a single | ||
// truncated TOTP response | ||
func (o *OATH) calculate(name string) (string, error) { | ||
func (o *OATH) CalculateOne(name string) (string, error) { | ||
|
||
var ( | ||
buf = make([]byte, 8) | ||
|
@@ -69,9 +79,9 @@ func (o *OATH) calculate(name string) (string, error) { | |
|
||
binary.BigEndian.PutUint64(buf, uint64(timestamp)) | ||
|
||
res, err := o.send(0x00, 0xa2, 0x00, 0x01, | ||
write(0x71, []byte(name)), | ||
write(0x74, buf), | ||
res, err := o.send(0x00, INST_CALCULATE, 0x00, RS_TRUNCATED_RESPONSE, | ||
write(TAG_NAME, []byte(name)), | ||
write(TAG_CHALLENGE, buf), | ||
) | ||
|
||
if err != nil { | ||
|
@@ -82,7 +92,7 @@ func (o *OATH) calculate(name string) (string, error) { | |
|
||
switch tv.tag { | ||
|
||
case 0x76: | ||
case TAG_TRUNCATED_RESPONSE: | ||
return otp(tv.value), nil | ||
|
||
default: | ||
|
@@ -95,9 +105,9 @@ func (o *OATH) calculate(name string) (string, error) { | |
|
||
} | ||
|
||
// calculateAll implements the "CALCULATE ALL" instruction to fetch all TOTP | ||
// CalculateAll implements the "CALCULATE ALL" instruction to fetch all TOTP | ||
// tokens and their codes (or a constant indicating a touch requirement) | ||
func (o *OATH) calculateAll() (map[string]string, error) { | ||
func (o *OATH) CalculateAll() (map[string]string, error) { | ||
|
||
var ( | ||
buf = make([]byte, 8) | ||
|
@@ -108,8 +118,8 @@ func (o *OATH) calculateAll() (map[string]string, error) { | |
|
||
binary.BigEndian.PutUint64(buf, uint64(timestamp)) | ||
|
||
res, err := o.send(0x00, 0xa4, 0x00, 0x01, | ||
write(0x74, buf), | ||
res, err := o.send(0x00, INST_CALCULATE_ALL, 0x00, RS_TRUNCATED_RESPONSE, | ||
write(TAG_CHALLENGE, buf), | ||
) | ||
|
||
if err != nil { | ||
|
@@ -120,15 +130,23 @@ func (o *OATH) calculateAll() (map[string]string, error) { | |
|
||
switch tv.tag { | ||
|
||
case 0x71: | ||
case TAG_NAME: | ||
names = append(names, string(tv.value)) | ||
|
||
case 0x7c: | ||
case TAG_TOUCH: | ||
codes = append(codes, touchRequired) | ||
|
||
case 0x76: | ||
case TAG_RESPONSE: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you elaborate the scenario in which we need to handle a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if there is a legitimate case for this, however it is included as a possible response in the spec. The result (once properly wrapping errors, as suggested) will be the same as not handling it, except with an error message that will be somewhat more helpful. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, understood. |
||
o.Debug("tag no full response: %x", tv.value) | ||
return nil, fmt.Errorf("unable to handle full response %x", tv.value) | ||
|
||
case TAG_TRUNCATED_RESPONSE: | ||
codes = append(codes, otp(tv.value)) | ||
|
||
case TAG_NO_RESPONSE: | ||
o.Debug("tag no response. Is HOTP %x", tv.value) | ||
return nil, fmt.Errorf("unable to handle HOTP response %x", tv.value) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Errors messages should generally be defined as private constants on the top of the file, prefixed with This patterns makes it easy to see what planned errors could be yielded by package. Regarding the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry. I think I understood what you meant here, but now I'm a little confused. It looks like all the error strings you used are at the top of Would it make sense to have those constants actually be This, however doesn't allow formatting as is done with I'm really a go hobbyist, so I'm not as familiar with the current best practices for errors in go as they seem to change every couple of months. 😄 |
||
|
||
default: | ||
return nil, fmt.Errorf(errUnknownTag, tv.tag) | ||
} | ||
|
@@ -147,9 +165,10 @@ func (o *OATH) calculateAll() (map[string]string, error) { | |
|
||
// otp converts a value into a (6 or 8 digits) one-time password | ||
func otp(value []byte) string { | ||
|
||
digits := value[0] | ||
digits := int(value[0]) | ||
code := binary.BigEndian.Uint32(value[1:]) | ||
// Limit code to a maximum number of digits | ||
code = code % uint32(math.Pow(10.0, float64(digits))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Und which circumstances would the code be > 8 digits? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had some TOTP that were supposed to be 6 digits (this was the value of |
||
// Format as string with a minimum number of digits, padded with 0s | ||
return fmt.Sprintf(fmt.Sprintf("%%0%dd", digits), code) | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,12 +13,15 @@ func (c code) Error() string { | |
|
||
if bytes.Equal(c, []byte{0x6a, 0x80}) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here you did not introduce constants? Could you also include the constant documentation links while at it (in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I only included the constants listed at the top of https://developers.yubico.com/OATH/YKOATH_Protocol.html Although this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On a similar note, rather than returning a string, I could make this return an |
||
return "wrong syntax" | ||
} else if bytes.Equal(c, []byte{0x69, 0x82}) { | ||
return "requires auth" | ||
} else if bytes.Equal(c, []byte{0x69, 0x84}) { | ||
return "no such object" | ||
} else if bytes.Equal(c, []byte{0x65, 0x81}) { | ||
return "generic error" | ||
} | ||
|
||
return fmt.Sprintf("unknown (% x)", []byte(c)) | ||
|
||
} | ||
|
||
// IsMore indicates more data that needs to be fetched | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
package ykoath | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer the constants to be private tbh - they do help readability but trigger linting advice when undocumented and public. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. I don't believe the API that this package exposes requires someone to access these either. I'll rename them. |
||
|
||
const ( | ||
// Instructions | ||
INST_PUT = 0x01 // Requires auth | ||
INST_DELETE = 0x02 // Requires auth | ||
INST_SET_CODE = 0x03 // Requires auth | ||
INST_RESET = 0x04 | ||
INST_LIST = 0xa1 // Requires auth | ||
INST_CALCULATE = 0xa2 // Requires auth | ||
INST_VALIDATE = 0xa3 | ||
INST_CALCULATE_ALL = 0xa4 // Requires auth | ||
INST_SEND_REMAINING = 0xa5 // Requires auth | ||
|
||
// Response size | ||
RS_FULL_RESPONSE = 0x00 | ||
RS_TRUNCATED_RESPONSE = 0x01 | ||
|
||
// Algorithms | ||
A_HMAC_SHA1 = 0x01 | ||
A_HMAC_SHA256 = 0x02 | ||
A_HMAC_SHA512 = 0x03 | ||
|
||
// OATH Types | ||
OT_HOTP = 0x10 | ||
OT_TOTP = 0x20 | ||
|
||
// Properties | ||
PROP_INCREASING = 0x01 | ||
PROP_REQUIRE_TOUCH = 0x02 | ||
|
||
// Tags | ||
TAG_NAME = 0x71 | ||
TAG_NAME_LIST = 0x72 | ||
TAG_KEY = 0x73 | ||
TAG_CHALLENGE = 0x74 | ||
TAG_RESPONSE = 0x75 | ||
TAG_TRUNCATED_RESPONSE = 0x76 | ||
TAG_NO_RESPONSE = 0x77 | ||
TAG_PROPERTY = 0x78 | ||
TAG_VERSION = 0x79 | ||
TAG_IMF = 0x7a | ||
TAG_ALGORITHM = 0x7b | ||
TAG_TOUCH = 0x7c | ||
|
||
// Mask | ||
MASK_ALGO = 0x0f | ||
MASK_TYPE = 0xf0 | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,12 @@ | ||
module github.com/yawn/ykoath | ||
|
||
go 1.15 | ||
|
||
require ( | ||
github.com/davecgh/go-spew v1.1.1 // indirect | ||
github.com/ebfe/scard v0.0.0-20190212122703-c3d1b1916a95 | ||
github.com/pkg/errors v0.8.1 | ||
github.com/stretchr/objx v0.1.1 // indirect | ||
github.com/stretchr/testify v1.3.0 | ||
golang.org/x/crypto v0.0.0-20201208171446-5f87f3452ae9 | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about this - this is already a public constant. What good does the additional constant do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. I had done this initially to minimize the changes, but a single constant should be sufficient.