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.

Sign up for GitHub

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

Jump to bottom

cli: add ssh command to securely connect with nodes over ssh #3568

Merged
merged 38 commits into from
Jan 30, 2025
Merged

cli: add ssh command to securely connect with nodes over ssh #3568

merged 38 commits into from
Jan 30, 2025

Conversation

miampf
Copy link
Contributor

@miampf miampf commented Jan 7, 2025 •
edited
Loading

Context

RFC 16 requires key derivation for the client and all nodes. This PR aims to implement that. For the main PR of this RFC, see #3557 (WIP).

Proposed change(s)

  • Add grpc functionality to request a CA key from the join service
    • This CA key is derived in the key service
    • The join client in the bootstrapper than gets the derived key in the IssueJoinTicketResponse

Additional info

  • The RFC specifies that the CA key should be derived directly from the master secret. This is not the case here. Instead, the key service requests a key from a KMS which in turn is used to derive the CA key. If the cKMS is used, the key sent by the KMS is derived using the master secret.

Checklist

@miampf miampf added the feature This introduces new functionality label Jan 7, 2025
@miampf miampf added this to the v2.21.0 milestone Jan 7, 2025
@miampf miampf requested a review from burgerdev January 7, 2025 16:08
@miampf
Copy link
Contributor Author

miampf commented Jan 7, 2025

Used the feature label instead of no changelog since this PR adds the constellation ssh subcommand for key derivation.

@netlify Netlify
Copy link

netlify bot commented Jan 7, 2025 •
edited
Loading

✅ Deploy Preview for constellation-docs canceled.

Name Link
🔨 Latest commit 7dbcc70
🔍 Latest deploy log https://app.netlify.com/sites/constellation-docs/deploys/679b60f40e365900074c119f

burgerdev
burgerdev reviewed Jan 8, 2025
View reviewed changes
cli/internal/cmd/ssh.go Outdated Show resolved Hide resolved
cli/internal/cmd/ssh.go Outdated Show resolved Hide resolved
cli/internal/cmd/ssh.go Outdated Show resolved Hide resolved
cli/internal/cmd/ssh.go Outdated Show resolved Hide resolved
cli/internal/cmd/ssh.go Outdated Show resolved Hide resolved
cli/internal/cmd/ssh.go Outdated Show resolved Hide resolved
joinservice/joinproto/join.proto Outdated Show resolved Hide resolved
keyservice/internal/server/server.go Outdated Show resolved Hide resolved
@miampf miampf force-pushed the miampf/key-derivation branch 4 times, most recently from 0bd9ac7 to ced42d5 Compare January 9, 2025 11:53
@miampf miampf requested a review from burgerdev January 9, 2025 14:54
@miampf miampf force-pushed the miampf/key-derivation branch 8 times, most recently from 932d7c5 to 215eae3 Compare January 14, 2025 15:41
burgerdev
burgerdev reviewed Jan 16, 2025
View reviewed changes
bootstrapper/internal/joinclient/joinclient_test.go Outdated Show resolved Hide resolved
joinservice/joinproto/join.proto Outdated Show resolved Hide resolved
internal/crypto/crypto_test.go Show resolved Hide resolved
cli/internal/cmd/ssh_test.go Outdated Show resolved Hide resolved
cli/internal/cmd/ssh_test.go Show resolved Hide resolved
@miampf miampf force-pushed the miampf/key-derivation branch 2 times, most recently from 981ebac to 10e9a73 Compare January 16, 2025 12:26
@miampf miampf requested a review from burgerdev January 16, 2025 12:27
@miampf miampf marked this pull request as ready for review January 16, 2025 12:27
@miampf miampf force-pushed the miampf/key-derivation branch from 10e9a73 to 6eb0158 Compare January 16, 2025 14:03
daniel-weisse
daniel-weisse requested changes Jan 16, 2025
View reviewed changes
Copy link
Member

@daniel-weisse daniel-weisse left a comment

Choose a reason for hiding this comment

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

Overall structure looks good to me.
Mostly notes on style and naming

bootstrapper/internal/joinclient/joinclient.go Outdated Show resolved Hide resolved
cli/internal/cmd/ssh.go Outdated Show resolved Hide resolved
cli/internal/cmd/ssh.go Outdated Show resolved Hide resolved
cli/internal/cmd/ssh.go Outdated Show resolved Hide resolved
internal/crypto/crypto_test.go Outdated Show resolved Hide resolved
joinservice/internal/server/server.go Outdated Show resolved Hide resolved
cli/internal/cmd/ssh.go Outdated Show resolved Hide resolved
e2e/miniconstellation/.terraform.lock.hcl Outdated Show resolved Hide resolved
cli/internal/cmd/ssh.go Outdated Show resolved Hide resolved
cli/internal/cmd/ssh.go Outdated Show resolved Hide resolved
@miampf miampf force-pushed the miampf/key-derivation branch from 6c935da to 57da20b Compare January 16, 2025 15:20
miampf added 26 commits January 30, 2025 12:21
@miampf
adjusted key derivation logic to happen in the join client
23b23d8
@miampf
regenerated protobuf definitions
570b735
@miampf
fixed tests
5053c45
@miampf
make key path a constant
6ec18eb
@miampf
also derive the key on the control plane nodes
d336d06
@miampf
refactoring
241d0cd
@miampf
please CI
0be301f
@miampf
added test for CA generation + use SeedSize constant
bee3f6c 
Previously, I just hard coded 256 as the key length that seeds the key
generation since it worked. Now, it uses ed25519.SeedSize (32) instead.
@miampf
wrote test for ssh subcommand
b5fddcd
@miampf
check return value of fh.Write in ssh_test.go
ea94ebd
@miampf
implemented suggestions
19181af
@miampf
move key derivation to joinservice instead of joinclient
f386dbc
@miampf
implement small changes
21c9cd6
@miampf
use cobra.Command
8577d23
@miampf
generate
f53e32d
@miampf
fix rebase issues
07a3309
@miampf
remove dot
f8f2490
@miampf
regenerate docs
cfa6ffc
@miampf
fix lock file
53c85c3
@miampf
fix joinservice unit test
3559fee
@miampf
gazelle generate
0e1e496
@miampf
fix other issues caused by variable renaming
3dfa405
@miampf
drop changes to tf lock completely
7fdd48f
@miampf
add license header to ssh_test.go
0cb3641
@miampf
generateKey -> writeCertificateForKey
70bc456
@miampf
regenerate protobuf files
7dbcc70
@miampf miampf force-pushed the miampf/key-derivation branch from 8f3a626 to 7dbcc70 Compare January 30, 2025 11:22
@github-actions GitHub Actions
Copy link
Contributor

Coverage report

Package Old New Trend
bootstrapper/internal/initserver 71.70% 67.90% ↘️
bootstrapper/internal/joinclient 87.00% 86.50% ↘️
cli/cmd 0.00% 0.00% 🚧
cli/internal/cmd 57.90% 58.10% ↗️
internal/constants 0.00% 0.00% 🚧
internal/crypto 69.60% 73.30% ↗️
joinservice/internal/server 78.20% 78.20% ↔️
joinservice/joinproto 0.00% 0.00% 🚧

@miampf miampf merged commit 706d1df into main Jan 30, 2025
11 checks passed
@miampf miampf deleted the miampf/key-derivation branch January 30, 2025 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daniel-weisse daniel-weisse daniel-weisse approved these changes

@burgerdev burgerdev burgerdev approved these changes

@3u13r 3u13r Awaiting requested review from 3u13r 3u13r is a code owner

Assignees
No one assigned
Labels
feature This introduces new functionality
Projects
None yet
Milestone
v2.21.0
Development

Successfully merging this pull request may close these issues.
3 participants
@miampf @daniel-weisse @burgerdev