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

PAYARA-1061 LetsEncrypt integration script #2727

Merged
merged 6 commits into from
Aug 10, 2018
Merged

PAYARA-1061 LetsEncrypt integration script #2727

merged 6 commits into from
Aug 10, 2018

Conversation

ratcashdev
Copy link
Contributor

@ratcashdev ratcashdev commented May 16, 2018

Requires certbot with the webroot plugin.

The idea is to deploy an empty war (as a directory, using in-place deployment) and provide that directory's path to certbot's webroot plugin to pass the challenge.
Script is written in python, because certbot itself requires python. As certbot works only under linux, so does this script.
Proper functionality (reloading the keystore and restarting the listener) requires b3bd6b0 and 83fb3c1

fixes #1047

@ratcashdev
Copy link
Contributor Author

@lprimak hi, can you please ask someone to look on this? thank you.

@lprimak
Copy link
Contributor

lprimak commented Jun 11, 2018

We will get to this. We are busy with a quarterly release right now.
Thanks for your patience

@mulderbaba
Copy link
Contributor

Jenkins test please

@mulderbaba mulderbaba added this to the Payara 5.183 milestone Jun 18, 2018
@payara-ci
Copy link
Contributor

Quick build and test passed!

@lprimak lprimak self-requested a review June 20, 2018 17:55
@lprimak
Copy link
Contributor

lprimak commented Jun 26, 2018

Im not sure that I am good person to review this, as I don't really know Python
Also, how do you run the script? Do you just go to the Payara install location and run it?
Any documentation / examples would be good so I or someone can at least test the functionality

@ratcashdev
Copy link
Contributor Author

ratcashdev commented Jun 26, 2018

@lprimak apologies. See the example here: #1047 (comment)

also, if you run the script without parameters, it will print the usage. Thus (on linux), you simply do:
letsencrypt.py or python3 letsencrypt.py

@MattGill98
Copy link
Contributor

Hi @ratcashdev,

Firstly, thank you for your contribution. I see that the certbot script is in Python, of which I'm not an expert. Even so, I don't personally see any functionality that couldn't be reproduced with Java in a way more native to our codebase. Am I incorrect in this assessment?

Kind regards,

Matt

@ratcashdev
Copy link
Contributor Author

Hi @MattGill98
Believe me, I am more of a JAVA guy too. And you're certainly correct in assuming there's nothing inside that could not be done in JAVA. I also understand the maintainability concerns given the lack of skillset.

In spite of all this, I believe python is much more appropriate for this very purpose. Admins and DevOps people (for whom this script is mainly for) can see the actual code, amend and adjust for their specific needs without recompilation/deployment.

Imagine the lost time for admins should they realize that the functionality is not exactly how it suits them? They'd need to clone the whole repo, figure out where exactly that code is, (learn java, maven, etc.), do their first build (which may take a couple of dozen minutes - if they succeed at all to begin with). And they would need to figure out which jar contains the result and how to replace it. Hours (if not days) just for a small change. I have experienced this first hand when I decided to get my hands dirty with letsencrypt integration and create my first asadmin command (see #2599 for example ).

And it's also worth noting that this would not be the only python file in the repo; I've counted over 100 python files, mostly related to configuration, packaging and some REST calls.

@lprimak
Copy link
Contributor

lprimak commented Jul 20, 2018

I am going to review this code this weekend/next week... sorry for the delay I was busy with other tasks

@ratcashdev
Copy link
Contributor Author

@lprimak no worries, take your time.

@MattGill98
Copy link
Contributor

Hi @ratcashdev,

Thank you for your response. I disagree with the precedence argument, since I think every case should be reviewed on it's own merit. However, your first argument is quite compelling. This in mind, I'm quite happy adding this python script if others seem to agree (which they seem to)!

Kind regards,

Matt

@lprimak lprimak changed the title LetsEncrypt integration script PAYARA-1061 LetsEncrypt integration script Jul 25, 2018
Copy link
Contributor

@lprimak lprimak left a comment

Choose a reason for hiding this comment

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

With certbot not installed, I am getting cryptic error message:

Lennys-MacBook-Pro:bin lprimak$ letsencrypt.py -d payara
Application deployed with name le.
Command deploy executed successfully.
/Users/lprimak/Documents/devel/Payara/appserver/distributions/payara/target/stage/payara5/glassfish/domains/domain1/config/keystore.jks
null
/etc/letsencrypt/live/payara/privkey.pem (No such file or directory)
Command add-pkcs8 failed.
Command undeploy executed successfully.

I would say there should be at least a check for this file existing, and possibly a command line argument of what the name is (hardcoded payara now) with payara being default

@lprimak
Copy link
Contributor

lprimak commented Jul 26, 2018

Also, it's assuming domain1 why not payara domain. Also, I am using separate domain directory outside Payara distribution, using --domaindir options to asadmin. This needs to be supported

@ratcashdev
Copy link
Contributor Author

@lprimak Thank you for the review and testing. Will have a look on this. So my understanding is that, the following changes are required:

  1. add the --domain-dir option.
  2. add the --domain-name option
  3. check for certbot's availability
    4.(Optional) echo back the comands run. This could be enabled with a --debug param, so that users will see which command went wrong.

@lprimak
Copy link
Contributor

lprimak commented Jul 26, 2018

@ratcashdev that's what I came up with so far.
Thank you!

@lprimak
Copy link
Contributor

lprimak commented Jul 28, 2018

the domain directory option should be --domaindir to be consistent with Payara,
and, also, isn't there already -d option in the script, and shouldn't it take the name from that option?

@ratcashdev
Copy link
Contributor Author

You're right. As a matter of fact you can specify multiple domain names. I've written it too long ago to remember.

@ratcashdev
Copy link
Contributor Author

Updated code. Start by ./letsencrypt.py -h, it will print the usage and arguments. Speaking about which, let me point out that it is necessary to distinguish between fully-qualified-domain-names (needed by certbot for the subject-alternative-names in the certificate) and payara domains (needed by asadmin).
except from the help message:

  -n DOMAIN_NAME, --domain-name DOMAIN_NAME
                        The FQDN of the domain(s) the certificate will be
                        bound too. You may use this arg multiple times.
  -p PAYARA_DOMAIN, --payara-domain PAYARA_DOMAIN
                        The name of the payara-domain where the certificate
                        will be uploaded.
  -d DOMAIN_DIR, --domain-dir DOMAIN_DIR
                        The directory where payara domains are defined.
                        Necessary to provide only when the domains are in non-
                        standard locations.

@lprimak
Copy link
Contributor

lprimak commented Jul 29, 2018

jenkins test

@lprimak
Copy link
Contributor

lprimak commented Jul 29, 2018

Can you please rename --domain-dir option to --domaindir to be consistent with Payara? Thanks

@ratcashdev
Copy link
Contributor Author

You're right. I am discussing a single 'dash' here, and it's really not worth it (even though customer scripts would not break, because, as I wrote, asadmin would keep --domaindir also in parallel for backward compatibility). So let's skip No3 and agree on --domaindir.

What about No2 from above?

@lprimak
Copy link
Contributor

lprimak commented Jul 31, 2018

I like —name to be consistent with payara asadmin commands still

@lprimak
Copy link
Contributor

lprimak commented Jul 31, 2018

I just want it to be consistent with the existing payara asadmin command to relieve users' cognitive load when learning a new command such as letsencrypt.

@ratcashdev
Copy link
Contributor Author

@lprimak would this be ok? Allowing both domaind-dir and domaindir.

  -c CERT_DOMAIN, --cert-domain CERT_DOMAIN
  -n NAME, --name NAME
  -d DOMAIN_DIR, --domain-dir DOMAIN_DIR, --domaindir DOMAIN_DIR

@lprimak
Copy link
Contributor

lprimak commented Aug 2, 2018

sure thing

@ratcashdev
Copy link
Contributor Author

@lprimak done with the changes. Please have a look.

@ratcashdev
Copy link
Contributor Author

ratcashdev commented Aug 2, 2018

It's important to note, that, by default, production domain also runs on port 8080. Thus, without some port mapping and configuration change in domain.xml the validation according to ACME (https://ietf-wg-acme.github.io/acme/draft-ietf-acme-acme.html see chapter 8.3, HTTP Challenge) will likely fail.

Maybe the script should warn about this...

@smillidge smillidge added the PR: DO NOT MERGE Don't merge PR until further notice label Aug 3, 2018
@smillidge
Copy link
Contributor

Please remove Do Not Merge label when this is agreed

@lprimak
Copy link
Contributor

lprimak commented Aug 3, 2018

It's important to note, that, by default, production domain also runs on port 8080.

Yes, I did see this. Alternatively there can be some port mapping / assignment done by the router.
It would be pretty easy / self-explanatory for the user to get done. I will write some docs / blog accompanying this script as well and there can be something there explaining this

@lprimak
Copy link
Contributor

lprimak commented Aug 5, 2018

jenkins test

@lprimak lprimak added 4:Ready and removed PR: DO NOT MERGE Don't merge PR until further notice labels Aug 5, 2018
@lprimak
Copy link
Contributor

lprimak commented Aug 5, 2018

Looks great!

Copy link
Contributor

@lprimak lprimak left a comment

Choose a reason for hiding this comment

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

Looks great now!

@payara-ci
Copy link
Contributor

Quick build and test passed!

@lprimak
Copy link
Contributor

lprimak commented Aug 5, 2018

Jenkins test

@payara-ci
Copy link
Contributor

Quick build and test passed!

@lprimak
Copy link
Contributor

lprimak commented Aug 5, 2018

@ratcashdev are you done with your changes? If you don't do anything in the next couple of days will merge it!

@ratcashdev
Copy link
Contributor Author

Hi @lprimak,
I made one last fix. Sorry about the confusion. I was considering the idea of temporarily changing listener-1's port to 80 (and then back to 8080 or whatever it was), but decided against it. That is to say, you're free to merge. There are no changes planned for the foreseeable future. And thanks for you patience, cooperation and help.

@lprimak
Copy link
Contributor

lprimak commented Aug 6, 2018

Oh, no problem. Happy for the contribution thanks!

@lprimak
Copy link
Contributor

lprimak commented Aug 6, 2018

jenkins test

@payara-ci
Copy link
Contributor

Quick build and test passed!

@lprimak lprimak merged commit 653ce0b into payara:master Aug 10, 2018
@johnmanko
Copy link
Contributor

It looks like the use case for this script is only for new non-Docker production installations. Perhaps it can be extended for use with Docker where the certs would be generated outside the container.

@ratcashdev
Copy link
Contributor Author

@johnmanko importing existing certs is already possible - this script already relies on that possibility.
See the asadmin add-pkcs8 command and its parameters. Also see the restart_listener method inside the script for an example how to restart a listener (and activating the newly imported keypair) in a running app-server.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Let's Encrypt Integration
7 participants