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

Improvements to letsencrypt_mgmt_profile.py #237

Merged
merged 21 commits into from
Aug 31, 2021

Conversation

patschi
Copy link
Contributor

@patschi patschi commented Jun 4, 2021

Notable changes:

  • Added support for Virtual Hosting scenarios (SNI)
  • Added possibility to update email contact
  • Added more comments/setup instructions
  • Added logging functionality to get at least some kind of output for debugging, if debug enabled
  • Fixed some small things, like error messages, default values, typos, etc
  • Fixed scenario where HTTP-to-HTTPS is enabled and token validation fails

Tested it with VH setup with SNI, RSA and ECDSA with a public-reachable test domain.

patschi added 14 commits June 3, 2021 23:17
Using tenant with "*" breaks the script as it searches for the tenant, but doesn't find it. As per AVI Python SDK when no value is specified (so using None), it will fallback to the default tenant for the specified user.
Certificate is being checked by default.
As its ensured that the VS is listening on port 80 anyway, we don't need to only apply the rule to port 80. The advantage is following: When HTTP-to-HTTPS redirect is enabled in the ApplicationProfile/parent VirtualHosting, the rule for port 80 won't apply and token validation fails. Without this, it also works on HTTPS (443) and token verification succeeds (LetsEncrypt follows redirect from :80 to :443 for token validation)
This was quite tricky, as child-VH VSs use vh_domain_name and cannot be identified via fqdn and don't have services object in the result. Therefore, when child VS detected, we do a slightly different search for VSs and set the services object from the parent VS. This way the later logic is mainly untouched.
@nikhilky nikhilky self-assigned this Jun 7, 2021
Copy link
Contributor

@nikhilky nikhilky left a comment

Choose a reason for hiding this comment

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

Thanks for these enhancements. I had few comments

cert_mgmt/letsencrypt_mgmt_profile.py Outdated Show resolved Hide resolved
cert_mgmt/letsencrypt_mgmt_profile.py Outdated Show resolved Hide resolved
Copy link

@chitr chitr left a comment

Choose a reason for hiding this comment

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

Lets log the Exceptions

cert_mgmt/letsencrypt_mgmt_profile.py Outdated Show resolved Hide resolved
cert_mgmt/letsencrypt_mgmt_profile.py Outdated Show resolved Hide resolved
@@ -172,44 +202,64 @@ def _do_request_avi(url, method, data=None, error_msg="Error"):
token = re.sub(r"[^A-Za-z0-9_\-]", "_", challenge['token'])
keyauthorization = "{0}.{1}".format(token, thumbprint)

# Update vs
# Get VSVIPs/VSs, based on FQDN
Copy link

@chitr chitr Jun 21, 2021

Choose a reason for hiding this comment

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

Break this flow to smaller functions based on related functionality.

if rsp['count'] == 0:
raise Exception("Could not find a VS with common name = {}".format(domain))

vs_uuid = rsp["results"][0]["uuid"]
print ("Found vs {} with fqdn {}".format(vs_uuid, domain))
# Check if the vs is servering on port 80

# Let's check if VS is enabled, otherwise challenge can never successfully complete.
Copy link

Choose a reason for hiding this comment

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

Lets move avi specific flow to separate function.(for next iteration )

@patschi
Copy link
Contributor Author

patschi commented Aug 21, 2021

I have had a chat with @nikhilky on this one. Main points:

  1. Made requested changes in commits above
  2. Breaking up the Avi specific flow to separate function is planned for a different PR

I have tested the current version, the last commit, working well on the latest 21.1.

There seems to be some issue when using multiple domains on one VS (basically using SANs) because of some restrictions of the search API functionality. I will check with Nikhil.

Copy link
Contributor

@nikhilky nikhilky left a comment

Choose a reason for hiding this comment

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

Discussed this PR with @chitr. The remaining items will be taken up in the next iteration.

@nikhilky nikhilky merged commit dbe24fb into avinetworks:master Aug 31, 2021
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.

None yet

3 participants