Cert server container #778

jmgrady · 2020-10-23T13:02:07Z

Implement the ability for the certmgr container to be able to create SSL certificates as a proxy for remote/offline servers:

update documentation, both certmgr/README.md and docstrings
configure Nginx server to serve dummy pages for NUCs (so that certbot's http challenge will succeed)
add scripts to certmgr container:
- aws.py for access to Amazon Web Services S3 bucket for certificates
- cert_proxy_server.py to implement the CertProxyServer class. CertProxyServer extends the LetsencryptCert class and adds creation/renewal of certficates as a proxy for offline devices.
remove deprecated bash shell versions of the certmgr scripts
move func.py to utils.py
add initial configuration files for the nuc group. These files will be used in a future PR.
merge in changes for backup and restore functions (affects some previous configuration items)
fixed generation of environment files for the backend, certmgr, and frontend containers - items that were integers instead of strings were not being included.
refactor docker_install role: make docker packages a list variable that can be overridden if needed.
update scripts/docker_setup.py

This change is

Fold the cert_server functionality into the letsencrypt mode so that there is not a race condition when renewing certificates if the cert_server is running in the same set of containers as the letsencrypt certmgr (expected deployment)

# Conflicts: # certmgr/scripts/entrypoint.py # certmgr/scripts/letsencryptcert.py

johnthagen · 2020-12-11T16:50:46Z

certmgr/scripts/aws.py, line 44 at r9 (raw file):

Previously, jmgrady (Jim Grady) wrote…

I have been able to reduce the occurrences significantly by making it an attribute of LetsEncryptCert. Is there a recommended pattern for creating module level constants? I found this reference: https://www.oreilly.com/library/view/python-cookbook/0596001673/ch05s16.html
Is this what you had in mind?

The convention if it's a constant value would be something like at the top of a module:

LETSENCRYPT_CONFIG_DIR = Path("/etc/letsencrypt")

And then other modules would import this constant:

from letsencrypt_cert import LETSENCRYPT_CONFIG_DIR

It could also be declared as a constant associated with a class and used like:

LetsEncryptCert.LETSENCRYPT_CONFIG_DIR

just depending on where you think the information would best live.

johnthagen · 2020-12-11T16:55:19Z

certmgr/scripts/letsencrypt_cert.py, line 32 at r11 (raw file):

        self.max_connect_tries: int = get_setting("MAX_CONNECT_TRIES")
        self.staging = get_setting("CERT_STAGING") != "0"
        self.le_dir = Path("/etc/letsencrypt")

Since this is always a constant value, it would be best to make it a "static" class attribute rather than an instance attribute. That way, you don't need a reference to self to access it (and it could even be accessed outside the class as well).

You can do this by moving it outside the constructor:

class LetsEncryptCert(BaseCert):
    """SSL Certificate class to create and renew certs from Let's Encrypt."""

    LE_DIR = Path("/etc/letsencrypt")

    def __init__ ...

Python allows you to access static class attributes with self for convenience if you are in a method, but this will allow this to be accessed from a staticmethod or without an instance to this class. This allows usages of it without signaling the need for (potentially) mutable access to the instance. (Since Python doesn't have the equivalent of a const method from C++ or taking self without mut as in Rust).

johnthagen · 2020-12-11T16:56:34Z

certmgr/scripts/cert_proxy_server.py, line 80 at r9 (raw file):

Previously, jmgrady (Jim Grady) wrote…

It uses self now! (Well, super() actually). In order to address all the instances of "/etc/letsencrypt" I made it an attribute of the LetsEncryptCert class so now this method looks up the directory in the parent class. Of course, if your method for creating module constants is better, then I will make this a static function.

I commented below, but yes ideally we should try to make things static when possible to limit their scope. Making a class attribute (as noted below) or a module attribute would be better for constants. It also uses a little bit less memory because each instance does't need to allocate a spot for the same constant.

johnthagen · 2020-12-11T16:57:06Z

certmgr/scripts/cert_proxy_server.py, line 84 at r11 (raw file):

        the proxy certificates are renewed.
        """
        renew_hook = Path(f"{super().get_le_dir()}/renewal-hooks/deploy/01_hook_push_certs_to_aws")

Replace with constant lookup (either static class attribute or module constant).

johnthagen · 2020-12-11T16:59:00Z

certmgr/scripts/entrypoint.py, line 18 at r9 (raw file):

Previously, jmgrady (Jim Grady) wrote…

I thought I would need Optional so that this will work:
    ...
    cert_obj = mode_choices.get(cert_mode, None)

    if cert_obj is not None:
in case someone tried to call something like mode_choices["foo"]

The .get() method on dicts is what can return None (or whatever default value you'd like.

So cert_obj in the example above would be Optional (since we are using None as the default), but mode_choices itself does not have Optional interior values.

johnthagen · 2020-12-11T17:00:03Z

certmgr/scripts/letsencrypt_cert.py, line 90 at r11 (raw file):

        os.system("certbot renew")

    def get_le_dir(self) -> Path:

This can be removed if we make a constant static class attribute for LE_DIR.

johnthagen · 2020-12-11T17:00:32Z

certmgr/scripts/letsencrypt_cert.py, line 152 at r11 (raw file):

        return False

    def update_renew_before_expiry(self, domain: str, renew_before_expiry_period: int) -> None:

This can be made staticmethod once we make a static LE_DIR.

johnthagen · 2020-12-11T17:03:41Z

docker_deploy/roles/combine_backup/templates/combine-restore.j2, line 107 at r9 (raw file):

Previously, jmgrady (Jim Grady) wrote…

I am using Jinja to fill in some of the machine specific details or current Combine configuration values. I can look into specifying these values as environment variables (esp. for a cron job) or as command line options - I've created Issue #860 for this.

One thing that makes reading the script a lot easier is that Atom has a package called shebang-set-grammar which uses the first line of the script to determine the programming language instead of the filename extension. I have looked for something like this for VS Code but most of the search results are about building my own extension. :-(

Yeah, Rider syntax highlights them fine actually. It's more trying to understand the "inputs" and control flow that was a bit more difficult for me at first.

johnthagen

Reviewed 6 of 8 files at r11.
Reviewable status: 34 of 36 files reviewed, 7 unresolved discussions (waiting on @jmgrady and @johnthagen)

johnthagen · 2020-12-11T21:15:17Z

certmgr/scripts/cert_proxy_server.py, line 84 at r11 (raw file):

Previously, johnthagen wrote…

Replace with constant lookup (either static class attribute or module constant).

LGTM

johnthagen · 2020-12-11T21:15:32Z

certmgr/scripts/entrypoint.py, line 18 at r9 (raw file):

Previously, johnthagen wrote…

The .get() method on dicts is what can return None (or whatever default value you'd like.

So cert_obj in the example above would be Optional (since we are using None as the default), but mode_choices itself does not have Optional interior values.

LGTM

johnthagen · 2020-12-11T21:15:45Z

certmgr/scripts/letsencrypt_cert.py, line 32 at r11 (raw file):

Previously, johnthagen wrote…

Since this is always a constant value, it would be best to make it a "static" class attribute rather than an instance attribute. That way, you don't need a reference to self to access it (and it could even be accessed outside the class as well).

You can do this by moving it outside the constructor:
class LetsEncryptCert(BaseCert):
    """SSL Certificate class to create and renew certs from Let's Encrypt."""

    LE_DIR = Path("/etc/letsencrypt")

    def __init__ ...
Python allows you to access static class attributes with self for convenience if you are in a method, but this will allow this to be accessed from a staticmethod or without an instance to this class. This allows usages of it without signaling the need for (potentially) mutable access to the instance. (Since Python doesn't have the equivalent of a const method from C++ or taking self without mut as in Rust).

LGTM

johnthagen · 2020-12-11T21:17:21Z

certmgr/scripts/letsencrypt_cert.py, line 90 at r11 (raw file):

Previously, johnthagen wrote…

This can be removed if we make a constant static class attribute for LE_DIR.

LGTM

johnthagen · 2020-12-11T21:17:29Z

certmgr/scripts/letsencrypt_cert.py, line 152 at r11 (raw file):

Previously, johnthagen wrote…

This can be made staticmethod once we make a static LE_DIR.

LGTM

johnthagen

Reviewed 1 of 8 files at r11, 1 of 5 files at r12, 6 of 8 files at r13, 2 of 2 files at r14.
Reviewable status: 36 of 37 files reviewed, all discussions resolved (waiting on @johnthagen)

jmgrady

Reviewable status: 36 of 37 files reviewed, all discussions resolved (waiting on @johnthagen)

certmgr/scripts/aws.py, line 44 at r9 (raw file):

Previously, johnthagen wrote…

The convention if it's a constant value would be something like at the top of a module:
LETSENCRYPT_CONFIG_DIR = Path("/etc/letsencrypt")
And then other modules would import this constant:
from letsencrypt_cert import LETSENCRYPT_CONFIG_DIR
It could also be declared as a constant associated with a class and used like:
LetsEncryptCert.LETSENCRYPT_CONFIG_DIR
just depending on where you think the information would best live.

Done.
I made it part of LetsEncryptCert.

certmgr/scripts/cert_proxy_server.py, line 80 at r9 (raw file):

Previously, johnthagen wrote…

I commented below, but yes ideally we should try to make things static when possible to limit their scope. Making a class attribute (as noted below) or a module attribute would be better for constants. It also uses a little bit less memory because each instance does't need to allocate a spot for the same constant.

Done.

certmgr/scripts/cert_proxy_server.py, line 84 at r11 (raw file):

Previously, johnthagen wrote…

Replace with constant lookup (either static class attribute or module constant).

Done.

certmgr/scripts/entrypoint.py, line 18 at r9 (raw file):

Previously, johnthagen wrote…

The .get() method on dicts is what can return None (or whatever default value you'd like.

So cert_obj in the example above would be Optional (since we are using None as the default), but mode_choices itself does not have Optional interior values.

That makes sense! Thank.
Done.

certmgr/scripts/letsencrypt_cert.py, line 32 at r11 (raw file):

Previously, johnthagen wrote…

Since this is always a constant value, it would be best to make it a "static" class attribute rather than an instance attribute. That way, you don't need a reference to self to access it (and it could even be accessed outside the class as well).

You can do this by moving it outside the constructor:
class LetsEncryptCert(BaseCert):
    """SSL Certificate class to create and renew certs from Let's Encrypt."""

    LE_DIR = Path("/etc/letsencrypt")

    def __init__ ...
Python allows you to access static class attributes with self for convenience if you are in a method, but this will allow this to be accessed from a staticmethod or without an instance to this class. This allows usages of it without signaling the need for (potentially) mutable access to the instance. (Since Python doesn't have the equivalent of a const method from C++ or taking self without mut as in Rust).

Done.

certmgr/scripts/letsencrypt_cert.py, line 90 at r11 (raw file):

Previously, johnthagen wrote…

This can be removed if we make a constant static class attribute for LE_DIR.

I don't think that it can. when certbot is run with the renew command, it checks all of the Let's Encrypt certificates that were issued to this server. If any of them are expiring soon (i.e. if there are less than renew_before_expiry days left before the certificates expire) then certbot will request a new certificate from Let's Encrypt. LE_DIR is not used.

certmgr/scripts/letsencrypt_cert.py, line 152 at r11 (raw file):

Previously, johnthagen wrote…

This can be made staticmethod once we make a static LE_DIR.

It still references self.email and self.staging.

johnthagen · 2020-12-12T17:48:24Z

certmgr/scripts/letsencrypt_cert.py, line 90 at r11 (raw file):

Previously, jmgrady (Jim Grady) wrote…

I don't think that it can. when certbot is run with the renew command, it checks all of the Let's Encrypt certificates that were issued to this server. If any of them are expiring soon (i.e. if there are less than renew_before_expiry days left before the certificates expire) then certbot will request a new certificate from Let's Encrypt. LE_DIR is not used.

(Yeah I think this comment used to be for a now-deleted line of code. I didn't mean to suggest removing this os.system call.

johnthagen · 2020-12-12T17:49:04Z

certmgr/scripts/letsencrypt_cert.py, line 152 at r11 (raw file):

Previously, jmgrady (Jim Grady) wrote…

It still references self.email and self.staging.

Similar comment. You addressed the original comment, but now that code has been removed, the comment has been solved.

johnthagen

Reviewed 1 of 1 files at r15.
Reviewable status: complete! all files reviewed, all discussions resolved

jmgrady added 10 commits October 20, 2020 17:52

Port certmgr entrypoint scripts to Python 3

80044fb

Merge remote-tracking branch 'origin/master' into port_certmgr_to_python

e8e9c91

Fix formatting and lint errors

4c5e9ed

Remove CERT_VERBOSE option for certmgr container

d53eb6b

Make Python scripts executable

8bc7cc0

Add aws-cli to image for future needs

5121833

Merge branch 'port_certmgr_to_python' into cert_server_container

fa12013

Add configuration items for cert_server behavior of certmgr container

4357f87

Setup web pages for NUC for certbot

073e435

jmgrady added deployment docker labels Oct 23, 2020

jmgrady requested a review from johnthagen October 23, 2020 13:02

jmgrady self-assigned this Oct 23, 2020

jmgrady added 4 commits October 23, 2020 09:14

Fix shebang

77f06cd

Make BaseCert an Abstract Base Class

01b5f3a

Fix type for mode_choices

b14b182

Updates from code review.

fb7eb49

johnthagen mentioned this pull request Oct 23, 2020

Port certmgr to python #777

Merged

jmgrady and others added 11 commits October 23, 2020 10:24

Fix type defs

d5318ae

Correct file formatting

e8c818c

Fix Lint errors

6e357dc

Merge branch 'port_certmgr_to_python' into cert_server_container

6eb9776

# Conflicts: # certmgr/scripts/entrypoint.py # certmgr/scripts/letsencryptcert.py

Fix renew method

2cc39cc

Remove trivial type definitions

5d5016a

Type

1f6abee

Merge branch 'master' into port_certmgr_to_python

773c123

Add comments for why we create self-signed cert and wait for webserver

4bb6076

Change cert_file to cert

a67d19e

Change List to Tuple

6cced8b

Changes from review

d15abbe

johnthagen requested changes Dec 11, 2020

View reviewed changes

johnthagen and others added 5 commits December 11, 2020 15:53

Enable type checking on certmgr directory

c6cecc4

Indent env var templates to improve readability

a2886ef

Add class constants and make functions static.

3581ede

Fix merge conflicts

365fc5e

Remove unneeded Optional

566be88

Remove unused import

dd35dcc

johnthagen approved these changes Dec 11, 2020

View reviewed changes

jmgrady commented Dec 11, 2020

View reviewed changes

johnthagen approved these changes Dec 12, 2020

View reviewed changes

jmgrady merged commit 006c7a5 into master Dec 12, 2020

jmgrady deleted the cert_server_container branch December 12, 2020 17:56

This was referenced Feb 26, 2021

Renew the SSL certificate for the demo server more frequently. #376

Closed

Need separate certificate for NUC targets #434

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cert server container #778

Cert server container #778

jmgrady commented Oct 23, 2020 •

edited

Loading

johnthagen commented Dec 11, 2020

johnthagen commented Dec 11, 2020

johnthagen commented Dec 11, 2020

johnthagen commented Dec 11, 2020

johnthagen commented Dec 11, 2020

johnthagen commented Dec 11, 2020

johnthagen commented Dec 11, 2020

johnthagen commented Dec 11, 2020

johnthagen left a comment

johnthagen commented Dec 11, 2020

johnthagen commented Dec 11, 2020

johnthagen commented Dec 11, 2020

johnthagen commented Dec 11, 2020

johnthagen commented Dec 11, 2020

johnthagen left a comment

jmgrady left a comment

johnthagen commented Dec 12, 2020

johnthagen commented Dec 12, 2020

johnthagen left a comment

Cert server container #778

Cert server container #778

Conversation

jmgrady commented Oct 23, 2020 • edited Loading

johnthagen commented Dec 11, 2020

johnthagen commented Dec 11, 2020

johnthagen commented Dec 11, 2020

johnthagen commented Dec 11, 2020

johnthagen commented Dec 11, 2020

johnthagen commented Dec 11, 2020

johnthagen commented Dec 11, 2020

johnthagen commented Dec 11, 2020

johnthagen left a comment

Choose a reason for hiding this comment

johnthagen commented Dec 11, 2020

johnthagen commented Dec 11, 2020

johnthagen commented Dec 11, 2020

johnthagen commented Dec 11, 2020

johnthagen commented Dec 11, 2020

johnthagen left a comment

Choose a reason for hiding this comment

jmgrady left a comment

Choose a reason for hiding this comment

johnthagen commented Dec 12, 2020

johnthagen commented Dec 12, 2020

johnthagen left a comment

Choose a reason for hiding this comment

jmgrady commented Oct 23, 2020 •

edited

Loading