Fix auth login error messages and messaging when witnessing a delegation with 0 valid keys #972

cyli · 2016-09-22T06:44:48Z

When I remove a key from a delegation that has a single key, then I try to witness that delegation, signing fails because there are no valid keys for that delegation. The error message was:

* fatal: signing keys not available, need 1 keys out of: - I've fixed the error message to say there are an insufficient number of valid signing keys for the threshold required, basically. We might want to move this error check up further to when we apply a witness change, since I think that might be the only time when we might have an invalid role.

Fixes #969 - that format string was in distribution, so I re-vendored. I also added logic to check if the login username/password is a terminal, otherwise fail.

Note: possibly we want to rebase this to 0.4.0 so that it can be added to 0.4.1

riyazdf

LGTM!

This is so much better

🐳 $ notary publish riyaz/private-repo
Pushing changes to riyaz/private-repo
Enter username: riyaz
Enter password:

* fatal: unauthorized: incorrect username or password

endophage · 2016-09-23T18:09:54Z

Is there an easy way to get Godep to exclude the distribution *.md and build files?

cyli · 2016-09-23T18:12:18Z

I can manually remove them - Godep tried to include the vendor dir :|

endophage · 2016-09-23T18:12:38Z

tuf/signed/errors.go

+	switch {
+	case len(e.MissingKeyIDs) < e.NeededKeys:
+		return fmt.Sprintf(
+			"cannot sign because while %d signatures are needed, an insufficient number of valid signing keys have been specified",


This reads a little verbosely. Maybe something like: "insufficient signing keys available. %d signatures are required but only %d keys are available"

Hmm... that implies that we are missing keys. We may not be missing any keys - it's just that the role doesn't specify enough keys for anyone to sign.

Maybe I should just say that the role is broken?

this case could be broken down further into len(e.MissingKeyIDS) + e.FoundKeys < e.NeededKeys and len(e.MissingKeyIDS) + e.FoundKeys >= e.NeededKeys right?

For len(e.MissingKeyIDS) + e.FoundKeys < e.NeededKeys: role is broken
For len(e.MissingKeyIDS) + e.FoundKeys >= e.NeededKeys: @endophage's suggestion of "insufficient signing keys available. %d signatures are required but only %d keys are available" This could be the base case in default

As per discussion IRL, this is basically just a role validity check. The only way we can get to this weird invalid state is with Witness, so we should just move this check to that functionality.

Signed-off-by: Ying Li <[email protected]>

…h errors Signed-off-by: Ying Li <[email protected]>

… be less verbose Signed-off-by: Ying Li <[email protected]>

…essed is invalid Signed-off-by: Ying Li <[email protected]>

cyli · 2016-09-26T20:32:28Z

@endophage @riyazdf sorry this took so long to update. Tried to fix some of the messaging around witnessing an invalid delegation role as suggested above.

riyazdf

LGTM, thanks for the update 👍

riyazdf · 2016-09-26T20:36:39Z

cmd/notary/integration_test.go

+	require.NoError(t, err)
+	_, err = runCommand(t, tempDir, "-s", server.URL, "witness", "-p", "gun", delgName)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "role does not specify enough valid signing keys to meet its required threshold")


awesome, thanks for adding this test 👍

endophage · 2016-09-27T20:16:05Z

cmd/notary/tuf.go

@@ -755,7 +757,8 @@ type passwordStore struct {
 }

 func (ps passwordStore) Basic(u *url.URL) (string, string) {
-	if ps.anonymous {
+	// if it's not a terminal, don't wait on input
+	if ps.anonymous || !terminal.IsTerminal(int(os.Stdin.Fd())) {


Shouldn't we be checking the input that was set on the command?

IRL: leave as is for now as we use os.Stdin elsewhere too. Issue open already for looking into this.

endophage

LGTM Including incoming version bump

Signed-off-by: Ying Li <[email protected]>

GordonTheTurtle added status/0-triage labels Sep 22, 2016

cyli modified the milestone: Notary 0.4.1 Sep 22, 2016

riyazdf approved these changes Sep 22, 2016

View reviewed changes

cyli force-pushed the fix-error-messages branch from 4265df6 to 272b194 Compare September 22, 2016 20:50

cyli changed the base branch from master to hotfix/0.4.1 September 22, 2016 20:50

endophage reviewed Sep 23, 2016

View reviewed changes

cyli force-pushed the fix-error-messages branch from 272b194 to 3352b85 Compare September 26, 2016 17:33

GordonTheTurtle added the dco/no label Sep 26, 2016

cyli added 3 commits September 26, 2016 10:34

Fix messaging when witnessing a role with no valid signing keys

e393de5

Signed-off-by: Ying Li <[email protected]>

Re-vendor distribution, which has better error messages for login aut…

25cd29c

…h errors Signed-off-by: Ying Li <[email protected]>

Updated error message when there are no possible signing keys left to…

84bb4ae

… be less verbose Signed-off-by: Ying Li <[email protected]>

cyli force-pushed the fix-error-messages branch from 3352b85 to d94ba8d Compare September 26, 2016 17:35

GordonTheTurtle removed the dco/no label Sep 26, 2016

Provide a more useful error message when a delegation role to be witn…

7772deb

…essed is invalid Signed-off-by: Ying Li <[email protected]>

cyli force-pushed the fix-error-messages branch from c270051 to 7772deb Compare September 26, 2016 19:30

riyazdf approved these changes Sep 26, 2016

View reviewed changes

endophage reviewed Sep 27, 2016

View reviewed changes

endophage approved these changes Sep 27, 2016

View reviewed changes

Bump notary version to 0.4.1 and update changelog

220cfb5

Signed-off-by: Ying Li <[email protected]>

cyli force-pushed the fix-error-messages branch from f293fd7 to 220cfb5 Compare September 27, 2016 20:38

cyli merged commit 30c07bd into notaryproject:hotfix/0.4.1 Sep 27, 2016

cyli deleted the fix-error-messages branch September 27, 2016 21:16

cyli mentioned this pull request Sep 30, 2016

Bump notary version to 0.4.2 moby/moby#27074

Merged

riyazdf mentioned this pull request Oct 4, 2016

Clean up error message for incorrect passphrase #969

Closed

riyazdf mentioned this pull request Oct 14, 2016

Cleanup error message for auth challenge #1007

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix auth login error messages and messaging when witnessing a delegation with 0 valid keys #972

Fix auth login error messages and messaging when witnessing a delegation with 0 valid keys #972

cyli commented Sep 22, 2016 •

edited

Loading

riyazdf left a comment

endophage commented Sep 23, 2016

cyli commented Sep 23, 2016

endophage Sep 23, 2016

cyli Sep 23, 2016

cyli Sep 23, 2016

riyazdf Sep 23, 2016

cyli Sep 23, 2016

cyli commented Sep 26, 2016

riyazdf left a comment

riyazdf Sep 26, 2016

endophage Sep 27, 2016

endophage Sep 27, 2016

endophage left a comment

Fix auth login error messages and messaging when witnessing a delegation with 0 valid keys #972

Fix auth login error messages and messaging when witnessing a delegation with 0 valid keys #972

Conversation

cyli commented Sep 22, 2016 • edited Loading

riyazdf left a comment

Choose a reason for hiding this comment

endophage commented Sep 23, 2016

cyli commented Sep 23, 2016

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

cyli commented Sep 26, 2016

riyazdf 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

endophage left a comment

Choose a reason for hiding this comment

cyli commented Sep 22, 2016 •

edited

Loading