Add VALIDATE calls #13

ViViDboarder · 2020-12-09T22:15:19Z

My Yubikey Neo is password protected and requires a call to validate the password before listing or performing other actions.

In order to make the magic numbers more readable I added all the ones I found on the Yubico site to constants.go.

Other smaller changes:

Export CALCULATE as CalculateOne and CALCULATE_ALL as CalculateAll
Add more error codes to codes.go

This is used as follows:

package main

import (
	"fmt"
	"git.iamthefij.com/iamthefij/slog"
	"github.com/yawn/ykoath"
	"golang.org/x/crypto/ssh/terminal"
	"syscall"
)

func main() {
	oath, err := ykoath.New()
	slog.FatalOnErr(err, "failed to initialize new oath")
	oath.Debug = slog.Debug

	defer oath.Close()

	s, err := oath.Select()
	slog.FatalOnErr(err, "failed to select oath")

	if s.Challenge != nil {
		fmt.Println("Passphrase? ")
		passphrase, err := terminal.ReadPassword(int(syscall.Stdin))
		slog.FatalOnErr(err, "failed reading passphrase")
		key := s.DeriveKey(string(passphrase))

		ok, err := oath.Validate(s, key)
		slog.FatalOnErr(err, "validation failed")
		if !ok {
			panic("could not validate")
		}
	}

	names, err := oath.List()
	slog.FatalOnErr(err, "failed to list names")

	for _, name := range names {
		slog.Log(name.Name)
	}

	otp, err := oath.Calculate(names[0].Name, ykoath.ErrorTouchCallback)
	slog.FatalOnErr(err, "Failed to retrieve otp")
	slog.Log(otp)
}

This allows authenticating with a password

yawn · 2020-12-10T07:51:17Z

Awesome PR @ViViDboarder, thanks a lot! Review will be done ~Monday.

I actually have no idea what happened here. This was working for me before and then suddenly wasn't and required this change.

ViViDboarder · 2020-12-18T20:13:12Z

For some reason I started getting 8 digit codes where I was supposed to get 6. I didn't change anything so I wonder if it has something to do with drivers or a dependency. To resolve it, I implemented a similar solution to yubikey-manager

ViViDboarder · 2021-01-04T17:02:43Z

Any chance of a review in the next week or so? I wrote a cli tool and an Alfred Workflow that depends on this. I can build and distribute binaries using replace in my go.mod for now, but it's easier for others to test and write patches if they can build directly.

yawn · 2021-01-07T10:06:31Z

Yeah, sorry about that. I'll do the review today / in the afternoon.

yawn

Thanks again for the PR, very nice work! Apart from the individual comments I have ~~two~~ three high-level questions:

Why the KDF in select.go?
Can you contribute changes to README as well (you made VALIDATE support a reality after all 👍 )
Can you contribute additional unit tests (please note that current suite will delete existing OATH tokens from your test key)?

Thanks again for the work & sorry for not getting back earlier!

yawn · 2021-01-07T13:23:34Z

algorithm.go

@@ -4,13 +4,13 @@ import "fmt"

 const (
 	// HmacSha1 describes a HMAC with SHA-1
-	HmacSha1 Algorithm = 0x01


Not sure about this - this is already a public constant. What good does the additional constant do?

That's a good point. I had done this initially to minimize the changes, but a single constant should be sufficient.

algorithm.go

yawn · 2021-01-07T13:28:32Z

constants.go

@@ -0,0 +1,49 @@
+package ykoath


I would prefer the constants to be private tbh - they do help readability but trigger linting advice when undocumented and public.

Good point. I don't believe the API that this package exposes requires someone to access these either. I'll rename them.

yawn · 2021-01-07T13:29:48Z

calculate.go

 			codes = append(codes, touchRequired)

-		case 0x76:
+		case TAG_RESPONSE:


Can you elaborate the scenario in which we need to handle a 0x75?

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.

Ok, understood.

select.go

yawn · 2021-01-07T13:42:40Z

validate.go

+func (o *OATH) Validate(s *Select, key []byte) (bool, error) {
+	algo, err := s.Hash()
+	if err != nil {
+		return false, err


Here another errors.Wrapf + constant would be preferred.

yawn · 2021-01-07T13:44:25Z

validate.go

+		return false, err
+	}
+
+	mac := hmac.New(algo, key)


Hm. If you really want to introduce a Hash function, you could potentially use the Sum / Sum256 / Sum512 functions from standard and return a slices on the array returned by them.

I'm not sure I follow.

I meant you could use the "sum" forms such as https://golang.org/pkg/crypto/sha256/#Sum256 - but I guess your way is more generic. Let's ignore this ...

yawn · 2021-01-07T13:44:39Z

validate.go

+
+	randChallenge := make([]byte, 8)
+	_, err = rand.Read(randChallenge)
+	if err != nil {


Same error remark as above.

yawn · 2021-01-07T13:45:03Z

validate.go

+	challenge := write(TAG_CHALLENGE, challengeSum)
+	_, err = o.send(0x00, INST_VALIDATE, 0x00, 0x00, response, challenge)
+	if err != nil {
+		return false, err


Same error remark as above.

ViViDboarder

Thanks for the feedback. Looks like a lot of cleanup on the constants and errors. I'll also update documentation to reflect the usage of the new functions within select.go.

Edit: Ugh. I guess that I didn't do responses properly. They show as replies above, but are duplicated here.

ViViDboarder · 2021-01-07T14:46:54Z

algorithm.go

@@ -4,13 +4,13 @@ import "fmt"

 const (
 	// HmacSha1 describes a HMAC with SHA-1
-	HmacSha1 Algorithm = 0x01


That's a good point. I had done this initially to minimize the changes, but a single constant should be sufficient.

ViViDboarder · 2021-01-07T14:49:15Z

calculate.go

 			codes = append(codes, touchRequired)

-		case 0x76:
+		case TAG_RESPONSE:


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.

ViViDboarder · 2021-01-07T14:51:49Z

calculate.go

 	code := binary.BigEndian.Uint32(value[1:])
+	// Limit code to a maximum number of digits
+	code = code % uint32(math.Pow(10.0, float64(digits)))


I had some TOTP that were supposed to be 6 digits (this was the value of digits), and they were returned as 8 digits. This will trim them to length of whatever the the value of digits is. This logic is the same as in yubikey-manager

ViViDboarder · 2021-01-07T14:54:25Z

code.go

@@ -13,12 +13,15 @@ func (c code) Error() string {

 	if bytes.Equal(c, []byte{0x6a, 0x80}) {


I only included the constants listed at the top of https://developers.yubico.com/OATH/YKOATH_Protocol.html

Although this 0x6a80 is not listed as a constant there, it appears to be consistent throughout the documentation. I'll take a look through and include those and make a note along with the documentation link.

ViViDboarder · 2021-01-07T14:55:29Z

code.go

@@ -13,12 +13,15 @@ func (c code) Error() string {

 	if bytes.Equal(c, []byte{0x6a, 0x80}) {


On a similar note, rather than returning a string, I could make this return an error for each of these. What do you think?

ViViDboarder · 2021-01-07T14:56:19Z

constants.go

@@ -0,0 +1,49 @@
+package ykoath


Good point. I don't believe the API that this package exposes requires someone to access these either. I'll rename them.

select.go

ViViDboarder · 2021-01-07T15:13:57Z

validate.go

+		return false, err
+	}
+
+	mac := hmac.New(algo, key)


I'm not sure I follow.

Rename constants add documentation

yawn · 2021-02-01T17:05:18Z

Hey @ViViDboarder - are the remaining issues clear? Should we proceed with this PR?

ViViDboarder · 2021-02-01T19:54:06Z

Thanks @yawn. I'm still a little confused on errors, which I think are the only outstanding items.

The way I understand error handling in Go right now (it seems to change frequently) is that a class should do something like this:

package example

import (
    "errors"
    "fmt"
)

var (
    ErrSomethingNotFound = errors.New("thing not found")
)

func CalledError() error {
    err := maybeHasError()
    if err != nil {
        return fmt.Errorf("error in this thing: %w", err)
    }

    return nil
}

func FindThing(p string) error {
    var b bool
    b = isThingFound(p)
    if !b {
        return fmt.Errorf("error finding %p: %w", p, ErrSomethingNotFound)
    }

    return nil
}

This allows some using the module and function to handle errors from findThing by using the wraps.

package main

import (
    "errors"
    "fmt"

    "example"
)

func main() {
    err = example.FindThing("foo")
    if errors.Is(err, example.ErrSomethingNotFound) {
        fmt.Println("Thing wasn't found")
    } else {
        panic(err)
    }
}

I can definitely do this, but it would be inconsistent with some of the others. I can refactor those as well, but maybe in a different patch.

yawn · 2021-02-02T10:09:49Z

As an alternative: just add the tests and I'll do the minor / cosmetic changes in your PR? Maybe easier?

ViViDboarder · 2021-02-02T17:20:06Z

That works for me.

Regarding the tests, I can definitely write some, but I do not have a spare Yubikey to test with. It also looks like I'll need to implement the SET CODE instruction that sets up authentication if a full test is to be written so that auth can be enabled, validated, and disabled.

yawn · 2021-02-02T18:15:52Z

I can totally live without SET CODE if you can substitute it with a ykman call. For the test environment that's fine. Spare Yubikey, hm - that's an issue of course :-)

ViViDboarder added 3 commits December 9, 2020 17:02

Add constants and replace some magic numbers

13ca424

Add validate functionality

4e940af

This allows authenticating with a password

Fix renames

3e1f718

Fix returning of 8 digit codes when they are supposed to be 6

0bb0699

I actually have no idea what happened here. This was working for me before and then suddenly wasn't and required this change.

yawn requested changes Jan 7, 2021

View reviewed changes

ViViDboarder commented Jan 7, 2021

View reviewed changes

ViViDboarder added 2 commits January 7, 2021 10:52

Review feedback

b083034

Rename constants add documentation

Update with improved spec supporting YK5

fd081cb

yawn force-pushed the master branch 3 times, most recently from 3c0dfa3 to 201009e Compare June 2, 2022 12:54

yawn self-assigned this Jan 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add VALIDATE calls #13

Add VALIDATE calls #13

ViViDboarder commented Dec 9, 2020

yawn commented Dec 10, 2020

ViViDboarder commented Dec 18, 2020

ViViDboarder commented Jan 4, 2021

yawn commented Jan 7, 2021

yawn left a comment •

edited

Loading

yawn Jan 7, 2021

ViViDboarder Jan 7, 2021

yawn Jan 7, 2021

ViViDboarder Jan 7, 2021

yawn Jan 7, 2021

ViViDboarder Jan 7, 2021

yawn Jan 8, 2021

yawn Jan 7, 2021

yawn Jan 7, 2021

ViViDboarder Jan 7, 2021

yawn Jan 8, 2021

yawn Jan 7, 2021

yawn Jan 7, 2021

ViViDboarder left a comment •

edited

Loading

ViViDboarder Jan 7, 2021

ViViDboarder Jan 7, 2021

ViViDboarder Jan 7, 2021

ViViDboarder Jan 7, 2021

ViViDboarder Jan 7, 2021

ViViDboarder Jan 7, 2021

ViViDboarder Jan 7, 2021

yawn commented Feb 1, 2021

ViViDboarder commented Feb 1, 2021

yawn commented Feb 2, 2021

ViViDboarder commented Feb 2, 2021

yawn commented Feb 2, 2021

		@@ -13,12 +13,15 @@ func (c code) Error() string {

		if bytes.Equal(c, []byte{0x6a, 0x80}) {

Add VALIDATE calls #13

Are you sure you want to change the base?

Add VALIDATE calls #13

Conversation

ViViDboarder commented Dec 9, 2020

yawn commented Dec 10, 2020

ViViDboarder commented Dec 18, 2020

ViViDboarder commented Jan 4, 2021

yawn commented Jan 7, 2021

yawn left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViViDboarder left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yawn commented Feb 1, 2021

ViViDboarder commented Feb 1, 2021

yawn commented Feb 2, 2021

ViViDboarder commented Feb 2, 2021

yawn commented Feb 2, 2021

yawn left a comment •

edited

Loading

ViViDboarder left a comment •

edited

Loading