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

Add option to select algorithm to generate ACME certificates #3319

Merged
merged 5 commits into from May 16, 2018

Conversation

mmatur
Copy link
Member

@mmatur mmatur commented May 14, 2018

What does this PR do?

This PR allow user to choose which key type to use to generate private key

Motivation

Fixes #2940

More

  • Added/updated tests
  • Added/updated documentation

@mmatur mmatur added this to the 1.7 milestone May 14, 2018
@mmatur mmatur requested a review from nmengin May 14, 2018 09:29
Copy link
Contributor

@dtomcej dtomcej left a comment

Choose a reason for hiding this comment

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

Awesome job.
LGTM
:shipit:

Copy link
Contributor

@nmengin nmengin left a 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.

keyType = acme.RSA4096
case "RSA8192":
keyType = acme.RSA8192
}
Copy link
Contributor

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.

Copy link
Member Author

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 {
Copy link
Contributor

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.

# Optional
# Default: "RSA4096"
#
# Available value : "EC256", "EC384", "RSA2048", "RSA4096", "RSA8192"
Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

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

😉

@nmengin nmengin changed the title Allow user to choose algo for certificates generation Add option to select algorithm to generate ACME certificates May 15, 2018
Copy link
Member

@juliens juliens left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@nmengin nmengin left a comment

Choose a reason for hiding this comment

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

LGTM 👏

@traefiker traefiker merged commit 68cc826 into traefik:master May 16, 2018
@mmatur mmatur deleted the feature/acme-ecdsa branch May 17, 2018 07:08
@denji denji mentioned this pull request Jul 13, 2018
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/acme kind/enhancement a new or improved feature. size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants