Page MenuHomePhabricator

Make the Conduit auth error for an unrecognized public key a little more useful
ClosedPublic

Authored by epriestley on Jul 17 2018, 11:06 PM.

Details

Summary

Ref T13168. This is just a small quality-of-life fix: we can disclose which public key we're talking about because public keys are public.

Test Plan
  • Hit public key error (through my own bumbling / not reading or following instructions). Specifically, I haven't associated the key with a device in Almanac.
  • Before: vague error.
  • After: more specific error with enough key material that I could grep for it.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Jul 17 2018, 11:06 PM
epriestley requested review of this revision.Jul 17 2018, 11:07 PM
yelirekim added a subscriber: yelirekim.EditedJul 19 2018, 2:04 AM

Isn't the end of the public key way more useful for human readability in general? Most of my keys generated via ssh-keygen (which I assume is the practically universal method?) end in an identifier that I can plausibly use to identify the key, like this one:

ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDzzrWAoWk0QFKtGz3860178qb5ue1cMSqrwYnBOJah7SXEUfwCOjXtvfvE3hyShS3SBdhm6qJZfSMngq7jO84Je1T7HU9FFjLg9HUPsfXaPtsojg4HhhN2n1dm+SysdmXScqIDb0g5/JWHenHTRh146t7ptP6+JzSHQGCWBzXsgXFCdlRrcrM+cUIGz9A0AIr9rZBgcwRtp8qK+TG9geA+Vcxf2Lw2T/GUS4nAeuT8KslAY6oC7ge2zT33VUhkvHyWV4iNlTdDHFneMvtdyXiCFa0oZwaQ7PDNw6Hrde7TDOkTi9Pppxawi8UYotsL7CzkeNIRWDNGXGTLyVsxHYBX mriley@mriley-C02TW3N0HTDD

(C02TW3N0HTDD is the very useful totally human interpretable name of my laptop)

amckinley accepted this revision.Jul 19 2018, 3:58 AM

Isn't the end of the public key way more useful for human readability in general?

I was going to say the same thing:

~ $ grep -r AAAAB3NzaC1 .ssh/ | wc -l
      18

64 leading characters is definitely unique enough though. Accepting as-is, but I agree with @yelirekim that the last 64 chars would be more useful.

This revision is now accepted and ready to land.Jul 19 2018, 3:58 AM

I think structuring it as The public key ending in "...<xyz>". is at least a little weird, and potentially confusing even if it's technically better in most cases.

The comment may also be arbitrarily long and non-unique. (When we generate keys, I think we currently just use the comment "Generated", which in a German localization would of course be "Gensherkeymatthaffenuntermazerhosenauomattermachinen", making the last 64 characters nonunique and nonidentifying.)

Perhaps best would be "aaa ... bbb" but we don't have a convenient UTF8-aware truncation process for that today.

(Practically, we don't have a "...xxx" truncator either, and we can't just strrev() the input and output because we don't have a UTF8-aware string reverser.)

I suppose reversing phutil_utf8v_combined() is a reasonably good approximation of a UTF8 reverser. It's probably not perfect (e.g., RTL marks should be inverted, I guess?) but if we were to implement phutil_utf8_strrev(), reversing phutil_utf8v_combined() would probably be a reasonable v0.

TUqTUo ɘlqmAxɘ

This revision was automatically updated to reflect the committed changes.