Skip to content
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

feat: add ssh key table #1515

Merged
merged 2 commits into from
Jan 10, 2025
Merged

feat: add ssh key table #1515

merged 2 commits into from
Jan 10, 2025

Conversation

kian99
Copy link
Contributor

@kian99 kian99 commented Jan 10, 2025

Description

This PR adds a database migration and gorm model for users' public SSH keys. I reviewed #1287 when making this PR

Some justifications for the types chosen:

  • bytea over text for the SSH public key. The Go ssh package has a PublicKey interface with a Marshal method returning a []byte. I expect this is what we will store in the database. The marshalled object also stores the key type.
  • A separate key comment column allows us to persist the comment (not stored in the object mentioned above).
  • No fingerprint column, the key fingerprint is a hash computed from the public key so we don't need to persist this separately.

Fixes JUJU-7348

Engineering checklist

  • Documentation updated
  • Covered by unit tests
  • Covered by integration tests

@kian99 kian99 requested a review from a team as a code owner January 10, 2025 09:39
created_at TIMESTAMP WITH TIME ZONE DEFAULT NOW(),
updated_at TIMESTAMP WITH TIME ZONE,
public_key BYTEA NOT NULL,
key_comment VARCHAR(255),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why 255? is there a hard limit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just a reasonable limit imo

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

255 bytes should be enough for everybody ;)

@kian99 kian99 requested a review from SimoneDutto January 10, 2025 11:49
Copy link
Collaborator

@alesstimec alesstimec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a question about the key fingerprint being stored

created_at TIMESTAMP WITH TIME ZONE DEFAULT NOW(),
updated_at TIMESTAMP WITH TIME ZONE,
public_key BYTEA NOT NULL,
key_comment VARCHAR(255),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

255 bytes should be enough for everybody ;)

id SERIAL PRIMARY KEY,
created_at TIMESTAMP WITH TIME ZONE DEFAULT NOW(),
updated_at TIMESTAMP WITH TIME ZONE,
public_key BYTEA NOT NULL,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think juju displays hexadecimal md5 key signature when you run juju ssh-keys so i'm wondering if it would be worth storing that as well so that we can do the lookup

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I was thinking about that too, but I don't expect users to have too many keys so we can just compute them from the public key

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but if it would enable us to do the lookup based on key fingerprint without requiring the user to specify the username explicitly..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I believe it would be incorrect to query the keys without the user specifying a username. It's a bit contrived but imagine I gave you my SSH key pair (or in the incredibly slim chance you happen to generate the same key). You could also upload the public key to the controller and then on login there are 2 keys that match.

It's the same principle as basic auth but without requiring a username. I don't know what the guarantees around uniqueness of SSH keys are (how long they are, their entropy, etc) but I'm fairly confident it would be wrong to query for keys without the "username" of the user attempting to login.

@kian99 kian99 merged commit f35d9c5 into canonical:v3 Jan 10, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants