feat: Allow master configuration for ssh key crypto system #10072

gt2345 · 2024-10-16T22:09:02Z

Ticket

Description

Add master config option to specify which algorithm to use to generate ssh key.

        security:
          ssh:
            key_type: "RSA" | "ECDSA" | "ED25519"

Default is "RSA" for backward compatibility.

This PR also removes the -p --passphrase option for det shell start, since it does not make sense and it's been broken since at least 01/2023. More discussion here.

Test Plan

Start master with one of the crypto system
Run det shell start
There should be a message similar to

shell (id: ab3068f6-f252-43f8-9b90-c3adf0985bda) is ready.                      
Warning: Permanently added 'ab3068f6-f252-43f8-9b90-c3adf0985bda' (ECDSA) to the list of known hosts.

if "ECDSA" is the crypto system.

Checklist

Changes have been manually QA'd
New features have been approved by the corresponding PM
User-facing API changes have the "User-facing API Change" label
Release notes have been added as a separate file under docs/release-notes/
See Release Note for details.
Licenses have been included for new code which was copied and/or modified from any external code

codecov · 2024-10-16T22:09:16Z

Codecov Report

Attention: Patch coverage is 67.27273% with 18 lines in your changes missing coverage. Please review.

Project coverage is 54.45%. Comparing base (7d6a1a7) to head (1690bb7).
Report is 39 commits behind head on main.

Files with missing lines	Patch %	Lines
master/pkg/ssh/ssh.go	75.00%	11 Missing ⚠️
master/internal/config/config.go	37.50%	5 Missing ⚠️
master/internal/api_shell.go	0.00%	1 Missing ⚠️
master/internal/core.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10072      +/-   ##
==========================================
+ Coverage   54.42%   54.45%   +0.02%     
==========================================
  Files        1262     1262              
  Lines      158886   158910      +24     
  Branches     3630     3630              
==========================================
+ Hits        86472    86527      +55     
+ Misses      72280    72249      -31     
  Partials      134      134

Flag	Coverage Δ
backend	`45.58% <67.27%> (+0.07%)`	⬆️
harness	`72.75% <ø> (+<0.01%)`	⬆️
web	`53.95% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
harness/determined/cli/shell.py	`23.36% <ø> (-0.06%)`	⬇️
master/internal/experiment.go	`33.70% <100.00%> (ø)`
master/pkg/tasks/task.go	`62.30% <ø> (ø)`
master/internal/api_shell.go	`33.79% <0.00%> (+1.76%)`	⬆️
master/internal/core.go	`6.23% <0.00%> (ø)`
master/internal/config/config.go	`58.06% <37.50%> (-0.57%)`	⬇️
master/pkg/ssh/ssh.go	`79.62% <75.00%> (+12.96%)`	⬆️

... and 4 files with indirect coverage changes

netlify · 2024-10-16T22:09:21Z

✅ Deploy Preview for determined-ui canceled.

Name	Link
🔨 Latest commit	`1690bb7`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/67198fcfb7137d0008cd8827

ioga

there's a typo in PR / commit template: EC25519 instead of ED25519. everything looks ok in the code though.

ioga · 2024-10-17T17:33:54Z

master/pkg/ssh/ssh.go

+
+func generateECDSAKey() (PrivateAndPublicKeys, error) {
+	var generatedKeys PrivateAndPublicKeys
+	privateKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)


Just as a note: I don't think there were any requests to make the curve size configurable, and P256 is the NIST recommendation, so this is fine. But one day we may have to update it or make configurable.

ioga · 2024-10-17T17:36:43Z

docs/release-notes/ssh-crypto-system.rst

+-  Master Configuration: Add support for crypto system configuration for ssh connection. ``security
+   -> crypto_system`` now accepts ``RSA``, ``ECDSA`` or ``ED25519``.


Looking through the release notes, we usually use period instead of ->, i.e. security.crypto_system.

Suggested change

- Master Configuration: Add support for crypto system configuration for ssh connection. ``security

-> crypto_system`` now accepts ``RSA``, ``ECDSA`` or ``ED25519``.

- Master Configuration: Add support for crypto system configuration for ssh connection. ``security.crypto_system`` now accepts ``RSA``, ``ECDSA`` or ``ED25519``.

ioga · 2024-10-17T17:40:23Z

master/internal/config/config.go

@@ -108,7 +117,8 @@ func DefaultConfig() *Config {
 				Group: "root",
 			},
 			SSH: SSHConfig{
-				RsaKeySize: 1024,
+				RsaKeySize:   1024,
+				CryptoSystem: RSACryptoSystem,


I was under impression we wanted to switch to ECDSA by default, because it's faster and compliant with FIPS140-2 requirements. I'd strongly suggest we do the switch, while leaving an option for the users to revert back to RSA if they really want.

I wonder if @mackrorysd or @rb-determined-ai want to say anything about this?

It is faster than reasonably-sized RSA keys. I never liked the 1024 key size choice when we made it years ago, because key-per-task took too long if you used 2048 keys. The argument I made was "let's not do one key per task then" And the argument made back to me was "1024 is fine if the tasks are not long lived" and I lost.

Certainly I don't think 1024 rsa keys is a good default by any measure.

I personally would default to ed25519 keys, but I think Ilia is right about what most customers would prefer.

Wait, actually, I do vote ed25519 because that's what ssh-keygen defaults to.

I don't know too much about ssh keys, but it looks like newer algorithms are better in terms of the balance of security and speed, so I put ed25519 down as default for now.

ed25519 is not in FIPS140, so the category of users who cares about that will have to be aware of it and change the config. ECDSA is still plenty fast and fit most requirements, which feels like the best fit for the default option.

Don't have a strong opinion here myself, but to repeat some information added by @dannysauer:

FWIW: FIPS 186-5, Digital Signature Standard includes ED25519 as of its official publication date Feb last year.
https://csrc.nist.gov/pubs/fips/186-5/final
This is helpful, because ECDSA depends on having a good random number generator, which is not always the case.

I would side with ED25519 here the more I've read about it. It seems to be the technically superior solution, it is getting accepted in other federal standard, etc.

master/internal/config/config.go

rb-determined-ai · 2024-10-18T13:01:13Z

master/internal/config/config.go

@@ -452,7 +462,8 @@ type SecurityConfig struct {

 // SSHConfig is the configuration setting for SSH.
 type SSHConfig struct {
-	RsaKeySize int `json:"rsa_key_size"`
+	RsaKeySize   int    `json:"rsa_key_size"`
+	CryptoSystem string `json:"crypto_system"`


crypto_system seems like an unexpected phrase for what it means. How about key_type?

For example, man ssh-keygen says:

The type of key to be generated is specified with the -t option. If invoked without any arguments, ssh-keygen will generate an Ed25519 key.

.circleci/real_config.yml

rb-determined-ai · 2024-10-22T15:36:54Z

master/internal/config/config.go

+	// RSACryptoSystem uses RSA.
+	RSACryptoSystem = "RSA"


Replace 'CryptoSystem' with 'KeyType' or something.

rb-determined-ai · 2024-10-22T15:39:10Z

docs/release-notes/ssh-crypto-system.rst

+-  Master Configuration: Add support for crypto system configuration for ssh connection.
+   ``security.key_type`` now accepts ``RSA``, ``ECDSA`` or ``ED25519``.


Worth mentioning the transition to ED25519 keys by default, and I'd also mention that ED25519 keys are faster and more secure than the old default, as well as being the default key type for ssh-keygen.

Mentioning that we're using more secure keys makes us look good, and mentioning that we're adopting the same default as ssh-keygen helps to reduce support burden by making the change sound like obvious rather than arbitrary.

rb-determined-ai · 2024-10-22T15:41:33Z

master/pkg/ssh/ssh_test.go

+}
+
+func TestSSHKeyGenerate(t *testing.T) {
+	keys, err := GenerateKey(config.SSHConfig{KeyType: config.RSACryptoSystem, RsaKeySize: 1024})


nit: for the unit test, can you use a keysize of 512?

It's absurdly small for security purposes, but it should exercise the codepaths just fine.

molinamelendezj

stamped from infra

stoksc

lgtm, only have non-blocking feedback (i.e., i'd appreciate it being addressed but don't explicitly expect to re-review)

stoksc · 2024-10-23T20:23:05Z

master/internal/api_user_intg_test.go

@@ -118,7 +118,7 @@ func setupAPITest(t *testing.T, pgdb *db.PgDB,
 				TaskContainerDefaults: model.TaskContainerDefaultsConfig{},
 				ResourceConfig:        *config.DefaultResourceConfig(),
 			},
-			taskSpec: &tasks.TaskSpec{SSHRsaSize: 1024},
+			taskSpec: &tasks.TaskSpec{SSHConfig: config.SSHConfig{RsaKeySize: 1024, KeyType: "RSA"}},


as a follow up: can/should we default to ed25519?

stoksc · 2024-10-23T20:24:50Z

master/internal/config/config.go

+	if t.KeyType != RSAKeyType && t.KeyType != ECDSAKeyType && t.KeyType != ED25519KeyType {
+		errs = append(errs, errors.New("Crypto system must be one of 'RSA', 'ECDSA' or 'ED25519'"))
+	}
+	if t.KeyType != RSAKeyType {


can we flip this and put this rsa specific stuff in an if t.KeyType == RSAKeyType block? written like this, it's easy for someone to add a check and the bottom of this func and not realize it's just for RSA.

stoksc · 2024-10-23T20:25:54Z

master/pkg/ssh/ssh.go

+	generatedKeys = PrivateAndPublicKeys{
+		PrivateKey: pem.EncodeToMemory(block),
+		PublicKey:  sshlib.MarshalAuthorizedKey(publicKey),
+	}
+
+	return generatedKeys, nil


nit

Suggested change

generatedKeys = PrivateAndPublicKeys{

PrivateKey: pem.EncodeToMemory(block),

PublicKey: sshlib.MarshalAuthorizedKey(publicKey),

}

return generatedKeys, nil

return PrivateAndPublicKeys{

PrivateKey: pem.EncodeToMemory(block),

PublicKey: sshlib.MarshalAuthorizedKey(publicKey),

}, nil

stoksc · 2024-10-23T20:26:03Z

master/pkg/ssh/ssh.go

+}
+
+func generateED25519Key() (PrivateAndPublicKeys, error) {
+	var generatedKeys PrivateAndPublicKeys


same nit here

The nit being that this could skip being declared at all? If so, I agree. I'd rather see

return PrivateAndPublicKeys{}, errors.Wrap(err, "unable to generate ED25519 private key")

and then no generatedKeys ever declared.

stoksc · 2024-10-23T20:50:17Z

master/pkg/ssh/ssh_test.go

+}
+
+func TestSSHKeyGenerate(t *testing.T) {
+	keys, err := GenerateKey(config.SSHConfig{KeyType: config.RSAKeyType, RsaKeySize: 512})


this test is so short it doesn't matter, but usually i'd recommend splitting a test like/using subtests with t.Run()/ using table driven tests this up since it really is three separate tests. it's easier for whoever has to fix one of them if it fails to read just the one test that failed... but i also doubt this test will ever fail. this comment is just FYI.

Big +1 to table-driven tests and subtests.

stoksc · 2024-10-23T20:51:26Z

i traded with max so he has time to review a pr of mine.

maxrussell · 2024-10-23T22:47:57Z

master/internal/config/config.go

+	// RSAKeyType uses RSA.
+	RSAKeyType = "RSA"
+	// ECDSAKeyType uses ECDSA.
+	ECDSAKeyType = "ECDSA"
+	// ED25519KeyType uses ED25519.
+	ED25519KeyType = "ED25519"


Idiomatically, these names should be KeyTypeRSA, KeyTypeECDSA, and KeyTypeED25519. A good example of this in the stdlib is net/http, where all the methods and statuses are named this way.

maxrussell · 2024-10-23T22:49:21Z

master/internal/config/config.go

+	if t.KeyType != RSAKeyType && t.KeyType != ECDSAKeyType && t.KeyType != ED25519KeyType {
+		errs = append(errs, errors.New("Crypto system must be one of 'RSA', 'ECDSA' or 'ED25519'"))
+	}
+	if t.KeyType != RSAKeyType {


maxrussell · 2024-10-23T22:53:18Z

master/pkg/ssh/ssh.go

+}
+
+func generateED25519Key() (PrivateAndPublicKeys, error) {
+	var generatedKeys PrivateAndPublicKeys


The nit being that this could skip being declared at all? If so, I agree. I'd rather see

return PrivateAndPublicKeys{}, errors.Wrap(err, "unable to generate ED25519 private key")

and then no generatedKeys ever declared.

maxrussell · 2024-10-23T22:54:49Z

master/pkg/ssh/ssh_test.go

+}
+
+func TestSSHKeyGenerate(t *testing.T) {
+	keys, err := GenerateKey(config.SSHConfig{KeyType: config.RSAKeyType, RsaKeySize: 512})


Big +1 to table-driven tests and subtests.

maxrussell

LGTM!

gt2345 added 2 commits October 15, 2024 14:10

add algorithm

5cbd0d2

fix header and ED25519

2a05eb1

gt2345 requested review from a team as code owners October 16, 2024 22:09

gt2345 requested a review from jesse-amano-hpe October 16, 2024 22:09

cla-bot bot added the cla-signed label Oct 16, 2024

gt2345 requested a review from rb-determined-ai October 16, 2024 22:09

gt2345 changed the title ~~Gt/475 ssh key alg~~ feat: Allow master configuration for ssh key crypto system Oct 16, 2024

add release note

b46f97f

determined-ci requested a review from a team October 16, 2024 22:37

determined-ci added the documentation Improvements or additions to documentation label Oct 16, 2024

remove passphrase when launching shell

ae88b02

gt2345 requested a review from a team as a code owner October 17, 2024 15:08

docs fmt

e444bc4

gt2345 requested a review from ioga October 17, 2024 15:55

ioga reviewed Oct 17, 2024

View reviewed changes

gt2345 added 3 commits October 17, 2024 12:48

update docs

5061abe

update docs

1a169d4

e2e

078d145

gt2345 requested a review from a team as a code owner October 17, 2024 19:26

gt2345 requested a review from molinamelendezj October 17, 2024 19:26

rb-determined-ai reviewed Oct 18, 2024

View reviewed changes

master/internal/config/config.go Outdated Show resolved Hide resolved

rb-determined-ai reviewed Oct 18, 2024

View reviewed changes

.circleci/real_config.yml Outdated Show resolved Hide resolved

rename to key_type

305b16e

determined-ci requested a review from a team October 18, 2024 20:50

gt2345 added 2 commits October 21, 2024 13:44

remove intg tests

e32501d

fmt docs

3c6475f

gt2345 added 3 commits October 21, 2024 16:24

unit test

76227ad

change default to ED25519

6442f9a

nolint:dogsled

6d53f1a

rb-determined-ai reviewed Oct 22, 2024

View reviewed changes

rb-determined-ai approved these changes Oct 22, 2024

View reviewed changes

update name and docs

c97313e

determined-ci requested a review from a team October 22, 2024 15:49

gt2345 requested review from corban-beaird and removed request for a team October 22, 2024 15:54

molinamelendezj approved these changes Oct 22, 2024

View reviewed changes

gt2345 requested review from maxrussell and removed request for corban-beaird October 22, 2024 17:30

stoksc approved these changes Oct 23, 2024

View reviewed changes

stoksc removed the request for review from maxrussell October 23, 2024 20:51

maxrussell requested changes Oct 23, 2024

View reviewed changes

comments

bf78880

gt2345 requested a review from maxrussell October 23, 2024 23:05

maxrussell approved these changes Oct 23, 2024

View reviewed changes

fmt

1690bb7

gt2345 merged commit fd9cd8a into main Oct 24, 2024
82 of 95 checks passed

gt2345 deleted the gt/475-ssh-key-alg branch October 24, 2024 00:56

thiagodallacqua-hpe pushed a commit that referenced this pull request Oct 28, 2024

feat: Allow master configuration for ssh key type (#10072)

79ebe68

thiagodallacqua-hpe pushed a commit that referenced this pull request Oct 28, 2024

feat: Allow master configuration for ssh key type (#10072)

65d9edc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow master configuration for ssh key crypto system #10072

feat: Allow master configuration for ssh key crypto system #10072

gt2345 commented Oct 16, 2024 •

edited

Loading

codecov bot commented Oct 16, 2024 •

edited

Loading

netlify bot commented Oct 16, 2024 •

edited

Loading

ioga left a comment

ioga Oct 17, 2024

ioga Oct 17, 2024

ioga Oct 17, 2024

gt2345 Oct 17, 2024

rb-determined-ai Oct 18, 2024

rb-determined-ai Oct 18, 2024

gt2345 Oct 21, 2024

ioga Oct 22, 2024

mackrorysd Oct 22, 2024

mackrorysd Oct 23, 2024

rb-determined-ai Oct 18, 2024

rb-determined-ai Oct 22, 2024

rb-determined-ai Oct 22, 2024

rb-determined-ai Oct 22, 2024

molinamelendezj left a comment

stoksc left a comment •

edited

Loading

stoksc Oct 23, 2024

stoksc Oct 23, 2024

maxrussell Oct 23, 2024

stoksc Oct 23, 2024

stoksc Oct 23, 2024

maxrussell Oct 23, 2024

stoksc Oct 23, 2024

stoksc Oct 23, 2024

maxrussell Oct 23, 2024

stoksc commented Oct 23, 2024

maxrussell Oct 23, 2024

maxrussell Oct 23, 2024

maxrussell Oct 23, 2024

maxrussell Oct 23, 2024

maxrussell left a comment

		- Master Configuration: Add support for crypto system configuration for ssh connection. ``security
		-> crypto_system`` now accepts ``RSA``, ``ECDSA`` or ``ED25519``.

		- Master Configuration: Add support for crypto system configuration for ssh connection.
		``security.key_type`` now accepts ``RSA``, ``ECDSA`` or ``ED25519``.

feat: Allow master configuration for ssh key crypto system #10072

feat: Allow master configuration for ssh key crypto system #10072

Conversation

gt2345 commented Oct 16, 2024 • edited Loading

Ticket

Description

Test Plan

Checklist

codecov bot commented Oct 16, 2024 • edited Loading

Codecov Report

netlify bot commented Oct 16, 2024 • edited Loading

✅ Deploy Preview for determined-ui canceled.

ioga 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

molinamelendezj left a comment

Choose a reason for hiding this comment

stoksc left a comment • edited Loading

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

stoksc commented Oct 23, 2024

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

maxrussell left a comment

Choose a reason for hiding this comment

gt2345 commented Oct 16, 2024 •

edited

Loading

codecov bot commented Oct 16, 2024 •

edited

Loading

netlify bot commented Oct 16, 2024 •

edited

Loading

stoksc left a comment •

edited

Loading