Implemented TOTP Secret Backend #2492

mymercurialsky · 2017-03-15T19:58:48Z

Implementation and test file for TOTP secret backend. This resolves #1197.

… template

…t conversion of period when generating passwords

This reverts commit fcd0309.

briankassouf

Minor things overall. Looking really good!

briankassouf · 2017-03-20T18:06:27Z

builtin/logical/totp/path_role_create.go

+	case "SHA512":
+		algorithm = otplib.AlgorithmSHA512
+	default:
+		algorithm = otplib.AlgorithmSHA1


If the default case is ever hit, it seems to me like something went wrong somewhere. Maybe it should return an error here instead.

briankassouf · 2017-03-20T18:07:01Z

builtin/logical/totp/path_role_create.go

+	case 6:
+		digits = otplib.DigitsSix
+	case 8:
+		digits = otplib.DigitsEight


We should add a default case that returns an error

briankassouf · 2017-03-20T18:09:33Z

builtin/logical/totp/path_roles.go

+			"Invalid key value: %s", err)), nil
+	}
+
+	if period < 0 {


period <= 0

briankassouf · 2017-03-20T18:10:15Z

builtin/logical/totp/path_roles.go

+	// Set optional parameters if neccessary
+	if period == 0 {
+		period = 30
+	}


This can be removed now that there is a default and if the above branch is changed to <=

briankassouf · 2017-03-20T18:11:55Z

builtin/logical/totp/path_roles.go

+	entry, err := logical.StorageEntryJSON("role/"+name, &roleEntry{
+		Key:          key,
+		Issuer:       issuer,
+		Account_Name: account_name,


Based on Go's best practices Account_Name should be: AccountName

briankassouf · 2017-03-20T18:12:14Z

builtin/logical/totp/path_roles.go

+type roleEntry struct {
+	Key          string `json:"key" mapstructure:"key" structs:"key"`
+	Issuer       string `json:"issuer" mapstructure:"issuer" structs:"issuer"`
+	Account_Name string `json:"account_name" mapstructure:"account_name" structs:"account_name"`


Account_Name -> AccountName

briankassouf · 2017-03-20T18:15:13Z

builtin/logical/totp/path_role_create.go

+
+	resp, err := &logical.Response{
+		Data: map[string]interface{}{
+			"token": totpToken,


Think the response should be key'ed as "code" rather than "token"?

jefferai · 2017-05-02T17:04:00Z

builtin/logical/totp/path_keys.go

+
+		//Read digits
+		digitsQuery, err := strconv.Atoi(urlQuery.Get("digits"))
+		if err == nil {


This should probably be if err != nil and error out if digits can't be parsed. Same with the period above.

Digits, Algorithm and Period are all optional values of a url. The error would come from strconv attempting to parse an empty string. Should I first do a check if the query is empty and if it is, skip that field and if it isn't parse it?

jefferai · 2017-05-02T17:04:29Z

builtin/logical/totp/path_keys.go

+	case 8:
+		keyDigits = otplib.DigitsEight
+	default:
+		return logical.ErrorResponse("the digit value can only be 6 or 8"), nil


s/digit/digits

jefferai · 2017-05-02T17:05:34Z

builtin/logical/totp/path_keys.go

+		return logical.ErrorResponse("the skew value must be 0 or 1"), nil
+	}
+
+	if qrSize <= 0 {


I don't think this should be required; if they want to only get a URL back (and the actual key separately) and skip the QR code we shouldn't force it. I'd have qrSize being 0 just disable QR code generation.

jefferai · 2017-05-02T17:07:53Z

builtin/logical/totp/path_keys.go

+		//Parse url
+		urlObject, err := url.Parse(inputURL)
+		if err != nil {
+			return logical.ErrorResponse("an error occured while parsing url string"), nil


...and what is that error? :-) You may not want to put it in the URL, but then at least log it (probably at debug or info level).

Do you mean log the error or log the URL? The URL might contain secret data that we don't want in the logs

I did mean log the error, although I guess depending on what that error is, if it spits out parts of the url it could log something secret. It may be worth returning the error to the caller though, presumably since they provided the URL they can be privy to parts of it in the error message.

Unless they're automated systems that then log the error automatically. So pick: turtles all the way down, or a potentially but not necessarily useful error message.

jefferai · 2017-05-02T17:08:41Z

builtin/logical/totp/path_keys.go

+			SecretSize:  uintKeySize,
+		})
+		if err != nil {
+			return logical.ErrorResponse("an error occured while generating a key"), nil


Same here, log the error or return it (in this case since the error wouldn't have the key value in it it's probably safe to just return) -- or both!

jefferai · 2017-05-02T17:08:49Z

builtin/logical/totp/path_keys.go

+		urlString := keyObject.String()
+		barcode, err := keyObject.Image(qrSize, qrSize)
+		if err != nil {
+			return logical.ErrorResponse("an error occured while generating a QR code image"), nil


Same comment...

jefferai · 2017-05-02T17:11:04Z

builtin/logical/totp/path_keys.go

+	uintSkew := uint(skew)
+	uintKeySize := uint(keySize)
+
+	var response logical.Response


Make this a pointer -- the default value will be nil. That way at the bottom you don't need an if block to switch on generate -- you just return response and if generate was false it'll automatically be nil.

jefferai

Looking much better! Some relatively minor comments.

jefferai · 2017-05-02T17:14:45Z

website/source/api/secret/totp/index.html.md

+
+- `name` `(string: <required>)` – Specifies the name of the key to create. This is specified as part of the URL.
+
+- `generate` `(bool: false)` – Specifies if a key is generated by Vault or if a key is being passed from another service.


I'd say "...if a key should be generated by Vault..."

jefferai · 2017-05-02T17:15:35Z

website/source/api/secret/totp/index.html.md

+
+- `key` `(string: <required - if generate is false and url is empty>)` – Specifies the master key used to generate a TOTP code. Only used if generate is false.
+
+- `issuer` `(string: "" <required - if generate is true and url is empty>)` – Specifies the name of the key’s issuing organization.


Same comment here about generate being true and url is empty not matching the actual code and the description of the parameters in the backend.

jefferai · 2017-05-02T17:16:01Z

website/source/api/secret/totp/index.html.md

+
+- `digits` `(int: 6)` – Specifies the number of digits in the generated TOTP code. This value can be set to 6 or 8.
+
+- `skew` `(int: 0)` – Specifies the number of delay periods that are allowed when validating a TOTP code. This value can be either 0 or 1. Only used if generate is true.


A reminder if you default to 1 to change this.

jefferai · 2017-05-02T17:17:19Z

website/source/api/secret/totp/index.html.md

+    https://vault.rocks/v1/totp/keys/my-key
+```
+
+### Sample Response


Please add some documentation here indicating that the returned QR code, if any, is a base64-encoded PNG.

jefferai · 2017-05-02T17:19:05Z

website/source/api/secret/totp/index.html.md

+}
+```
+
+### Embedding Barcodes in HTML


Just saw this; I think you can add the note above, and then just move this text up there along with it, e.g. "If a QR code is returned, it is base64-formatted PNG bytes. You can embed it in a web page by including..."

jefferai · 2017-05-02T17:54:48Z

builtin/logical/totp/path_keys.go

+func pathKeys(b *backend) *framework.Path {
+	return &framework.Path{
+		Pattern: "keys/" + framework.GenericNameRegex("name"),
+		Fields: map[string]*framework.FieldSchema{


Please add a boolean field (exported probably) that defaults to true and that controls whether or not a key that is generated is actually returned as a URL/QR code. The use-case is if you want to give one Vault user access to fetch a code, another access to validate the codes, but you don't ever actually want the key to be exposed.

A neat enhancement in this case might be to have the algorithm default to SHA256 if this is false, because if you know the key is never used anywhere else, you don't have to worry about compatibility issues.

… up error reporting, changed default skew value, updated documentation and added additional tests

…option

mymercurialsky · 2017-05-03T19:55:18Z

I made another round of changes based on Jeff's comments.

jefferai · 2017-05-03T22:33:21Z

builtin/logical/totp/path_code.go

+		return logical.ErrorResponse(fmt.Sprintf("unknown key: %s", name)), nil
+	}
+
+	valid, err := totplib.ValidateCustom(code, key.Key, time.Now(), totplib.ValidateOpts{


You're not checking this error -- if there is an error here it should bail out.

jefferai · 2017-05-03T22:34:15Z

builtin/logical/totp/path_code.go

+	}
+
+	// Return the secret
+	resp, err := &logical.Response{


s/resp, err :=/return/ ? There's really no reason to define both of these only to return them, just return them directly.

jefferai

🚢 🕺 👯‍♂️ 👯 💃

Oh, sorry -- after those two comments are fixed that is!

… validating codes and clarified documentation for generate parameters in path_keys

mymercurialsky · 2017-05-04T01:06:41Z

I fixed the two issues you mentioned. One note, one of the errors that the validation code generates is if the code is of invalid length (doesn't match digits). I set it to ignore that particular error but I can change it if you want. My thought was that if a code is invalid the backend should just return the false value in the return data.

jefferai · 2017-05-04T02:59:03Z

That sounds fine!

briankassouf

Looks great to me! Fantastic work!

jefferai · 2017-05-04T18:19:12Z

@mymercurialsky So good...seriously stoked about this. Thanks for all of the hard work and perseverance!

vsivsi · 2017-05-08T22:03:42Z

Fantastic, now that this is in place, can we expect that using TOTP as a MFA type for userpass authentication won't be far behind? Like was proposed long ago:

#132
#812

Would be a great addition!

jefferai · 2017-05-10T14:30:15Z

@vsivsi No -- that's something else entirely. It is on the roadmap, though.

vsivsi · 2017-05-10T17:30:35Z

Right, but this update contains a lot of functionality useful for implementing the MFA auth code, no?. Anyway, glad to hear it is on the roadmap because it is necessary.

jefferai · 2017-05-10T20:41:36Z

@vsivsi No, it's entirely separate. Code that can be used for MFA has been in a branch for a long time, but it's been waiting on some other core changes to happen.

mymercurialsky added 26 commits March 4, 2017 21:13

Initialized basic outline of TOTP backend using Postgresql backend as…

f9f4cd0

… template

Updated TOTP backend.go's structure and help string

77a1694

Updated TOTP path_roles.go's structure and help strings

b04ab7e

Updated TOTP path_role_create.go's structure and help strings

f64f5b9

Fixed typo in path_roles.go

49f3c0c

Merge branch 'master' of https://github.com/hashicorp/vault

3ede55c

Fixed errors in path_role_create.go and path_roles.go

f19b57f

Added TOTP secret backend information to cli commands

ff8fe83

Fixed build errors in path_roles.go and path_role_create.go

4a222fd

Changed field values of period and digits from uint to int, added uin…

44b681b

…t conversion of period when generating passwords

Initialized TOTP test file based on structure of postgresql test file

092e797

Added enforcement of input values

7a97efa

Merge branch 'master' of https://github.com/hashicorp/vault

dd96080

Added otp library to vendor folder

d2b07be

Added test steps and cleaned up errors

fb98943

Modified read credential test step, not working yet

1a7fe62

Merge branch 'master' of https://github.com/hashicorp/vault

8d59054

Use of vendored package not allowed - Test error

3269056

Removed vendor files for TOTP library

fcd0309

Revert "Removed vendor files for TOTP library"

6136a0f

This reverts commit fcd0309.

Hopefully fixed vendor folder issue with TOTP Library

2a122fc

Added additional tests for TOTP backend

3825e48

Cleaned up comments in TOTP backend_test.go

07d5a2f

Added default values of period, algorithm and digits to field schema

9d2b862

Changed account_name and issuer fields to optional

0010233

Removed MD5 as a hash algorithm option

bf3d7a0

briankassouf suggested changes Mar 20, 2017

View reviewed changes

Implemented requested pull request changes

6dadf12

briankassouf added this to the 0.7.1 milestone Mar 20, 2017

briankassouf requested a review from jefferai March 21, 2017 17:15

jefferai reviewed May 2, 2017

View reviewed changes

mymercurialsky added 2 commits May 2, 2017 20:11

Added exported variable and ability to disable QR generation, cleaned…

fbabadd

… up error reporting, changed default skew value, updated documentation and added additional tests

Updated API documentation to inlude to exported variable and qr size …

0023345

…option

jefferai reviewed May 3, 2017

View reviewed changes

jefferai approved these changes May 4, 2017

View reviewed changes

Cleaned up return statements in path_code, added error handling while…

ecff38e

… validating codes and clarified documentation for generate parameters in path_keys

briankassouf approved these changes May 4, 2017

View reviewed changes

briankassouf merged commit 461d658 into hashicorp:master May 4, 2017


		- `name` `(string: <required>)` – Specifies the name of the key to create. This is specified as part of the URL.

		- `generate` `(bool: false)` – Specifies if a key is generated by Vault or if a key is being passed from another service.


		- `key` `(string: <required - if generate is false and url is empty>)` – Specifies the master key used to generate a TOTP code. Only used if generate is false.

		- `issuer` `(string: "" <required - if generate is true and url is empty>)` – Specifies the name of the key’s issuing organization.


		- `digits` `(int: 6)` – Specifies the number of digits in the generated TOTP code. This value can be set to 6 or 8.

		- `skew` `(int: 0)` – Specifies the number of delay periods that are allowed when validating a TOTP code. This value can be either 0 or 1. Only used if generate is true.

Implemented TOTP Secret Backend #2492

Implemented TOTP Secret Backend #2492

Conversation

mymercurialsky commented Mar 15, 2017 • edited by jefferai Loading

briankassouf left a 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

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

jefferai left a 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

mymercurialsky commented May 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jefferai left a comment • edited Loading

Choose a reason for hiding this comment

mymercurialsky commented May 4, 2017

jefferai commented May 4, 2017

briankassouf left a comment

Choose a reason for hiding this comment

jefferai commented May 4, 2017

vsivsi commented May 8, 2017

jefferai commented May 10, 2017

vsivsi commented May 10, 2017

jefferai commented May 10, 2017

mymercurialsky commented Mar 15, 2017 •

edited by jefferai

Loading

jefferai left a comment •

edited

Loading