-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Add option to select algorithm to generate ACME certificates #3319
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome job.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks for this very useful PR @mmatur .
I just have few suggestions.
provider/acme/account.go
Outdated
keyType = acme.RSA4096 | ||
case "RSA8192": | ||
keyType = acme.RSA8192 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT to you manage the default
case?
Maybe just add a warning/error message to indicate that the default algorithm will be used because the given one is not managed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You right. I will fix
acme/account.go
Outdated
@@ -103,6 +107,25 @@ func (a *Account) GetPrivateKey() crypto.PrivateKey { | |||
return nil | |||
} | |||
|
|||
func getKeyType(value string) acme.KeyType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function seems to be a duplicate of provider/acme/account.go
.
Maybe can you make the function in provider/acme/account.go
public
and delete this one?
This kind of management is already used in ACME.
docs/configuration/acme.md
Outdated
# Optional | ||
# Default: "RSA4096" | ||
# | ||
# Available value : "EC256", "EC384", "RSA2048", "RSA4096", "RSA8192" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/value/values
@@ -9,15 +9,15 @@ readonly doc_file=$basedir"/docker-compose.yml" | |||
down_environment() { | |||
echo "STOP Docker environment" | |||
! docker-compose -f $doc_file down -v &>/dev/null && \ | |||
echo "[ERROR] Impossible to stop the Docker environment" && exit 11 | |||
echo "[ERROR] Unable to stop the Docker environment" && exit 11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😉
1491bca
to
7c985af
Compare
7c985af
to
f83954c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👏
What does this PR do?
This PR allow user to choose which key type to use to generate private key
Motivation
Fixes #2940
More