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

Increase rsa key size to 3072 #348

Merged
merged 8 commits into from
Oct 13, 2023

Conversation

dimityrmirchev
Copy link
Member

@dimityrmirchev dimityrmirchev commented Oct 13, 2023

What this PR does / why we need it:
Gardener uses 4096 bits to generate ssh keys. Let's use the same sizing here as well. See #348 (comment)

Similar to gardener/gardener#8635

https://github.com/gardener/gardener/blob/3e9f4819aa8ab5badacf76f8bb7c1dae31192bf3/pkg/operation/botanist/secrets.go#L255
Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

`ssh` now uses 3072 bit keys

@dimityrmirchev dimityrmirchev requested a review from a team as a code owner October 13, 2023 07:51
@gardener-robot gardener-robot added needs/review Needs review size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) labels Oct 13, 2023
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 13, 2023
Change the default ginkgo.Eventually timeout to 5sec and polling interval to 200ms
@gardener-robot gardener-robot added size/s Size of pull request is small (see gardener-robot robot/bots/size.py) and removed size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) labels Oct 13, 2023
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 13, 2023
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 13, 2023
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 13, 2023
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 13, 2023
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 13, 2023
@dimityrmirchev
Copy link
Member Author

I did some additional testing and it seems that generating a 4096 bit RSA key on a client machine will probably not be the most enjoyable experience since it takes significantly more time in comparison to a 2048 bit RSA key. I propose that we use a 3072 bit key since this also probably satisfies the recommendations from different security agencies and will be faster to generate. In a later stage we might consider replacing RSA with ECDSA which is significantly faster to generate keys due the smaller bit sizes providing the same security strength.

import (
	"crypto/ecdsa"
	"crypto/elliptic"
	"crypto/rand"
	"crypto/rsa"
	"testing"
)

func BenchmarkRSA2048(b *testing.B) {
	for i := 0; i < b.N; i++ {
		_, err := rsa.GenerateKey(rand.Reader, 2048)
		if err != nil {
			b.Fatalf("error occurred")
		}
	}
}

func BenchmarkRSA3072(b *testing.B) {
	for i := 0; i < b.N; i++ {
		_, err := rsa.GenerateKey(rand.Reader, 3072)
		if err != nil {
			b.Fatalf("error occurred")
		}
	}
}

func BenchmarkRSA4096(b *testing.B) {
	for i := 0; i < b.N; i++ {
		_, err := rsa.GenerateKey(rand.Reader, 4096)
		if err != nil {
			b.Fatalf("error occurred")
		}
	}
}

func BenchmarkECDSA256(b *testing.B) {
	for i := 0; i < b.N; i++ {
		_, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
		if err != nil {
			b.Fatalf("error occurred")
		}
	}
}
go test -bench=.  -benchtime=10s
goos: darwin
goarch: arm64
pkg: github.com/test/play
BenchmarkRSA2048-12                  134          90775602 ns/op
BenchmarkRSA3072-12                   36         374908398 ns/op
BenchmarkRSA4096-12                   15        1335982369 ns/op
BenchmarkECDSA256-12             1000000             11136 ns/op
PASS
ok      github.com/test/play    72.649s

@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 13, 2023
@dimityrmirchev dimityrmirchev changed the title Increase rsa key size to 4096 Increase rsa key size to 3072 Oct 13, 2023
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 13, 2023
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 13, 2023
@petersutter petersutter added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 13, 2023
@gardener-robot-ci-2 gardener-robot-ci-2 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 13, 2023
Copy link
Member

@petersutter petersutter left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/review Needs review labels Oct 13, 2023
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 13, 2023
@petersutter
Copy link
Member

/cla

@gardener-robot
Copy link

@petersutter I reached out successfully to the cla-assistant to recheck this pull request.

@petersutter petersutter merged commit 3b57cbf into gardener:master Oct 13, 2023
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Oct 13, 2023
@petersutter petersutter added the area/ipcei IPCEI (Important Project of Common European Interest) label Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ipcei IPCEI (Important Project of Common European Interest) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) size/s Size of pull request is small (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants