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

security/acme-client: Add domains.google support #3429

Closed
wants to merge 1 commit into from

Conversation

loosecannon93
Copy link

Adds support for Google Domains DNS-01 ACME challenge API added to latest upstream acme.sh. This is separate from the Google Cloud API.

https://domains.google/learn/gts-acme/

See acmesh-official/acme.sh#4542

This should address #3408 but ultimately depends on acme.sh cutting a new release with the above pull request included, so I assume would not be able to be merged until then.

@loosecannon93 loosecannon93 marked this pull request as ready for review May 10, 2023 06:49
@fraenki fraenki self-assigned this May 10, 2023
@fraenki
Copy link
Member

fraenki commented May 10, 2023

This should address #3408 but ultimately depends on acme.sh cutting a new release with the above pull request included, so I assume would not be able to be merged until then.

Exactly. I'll monitor acme.sh releases and will merge it as soon as possible.

@fraenki fraenki added feature Adding new functionality upstream Third party issue labels May 10, 2023
Adds support for Google Domains DNS-01 ACME challenge API added to
latest upstream acme.sh. This is separate from the Google Cloud API.

https://domains.google/learn/gts-acme/

See acmesh-official/acme.sh#4542
@dmurphynj
Copy link

Hi team, found an issue with this commit.

I applied it using "opnsense-patch -c plugins e10a960"
and continued to have errors when issuing a cert:
"AcmeClient: challenge type not supported: dns_googledomains (0ae7b9bb-f72c-47cd-9d78-3bc51b86d235)"

Long story short, it appears that "security/acme-client/src/opnsense/mvc/app/library/OPNsense/AcmeClient/LeValidation/DnsGoogleDomains.php"
should be renamed to:
"security/acme-client/src/opnsense/mvc/app/library/OPNsense/AcmeClient/LeValidation/DnsGoogledomains.php"

There is a case-sensitivity issue in LeValidationFactory.php, lines 69-70:

// Convert to PascalCase
$val_name = str_replace(' ', '', ucwords(str_replace(array('-', '_'), ' ', $search_name)));

returns as "DnsGoogledomains" after parsing.

Simply renaming DnsGoogleDomains.php to DnsGoogledomains.php seems to work just fine.

os-acme-client plugin 3.17
OPNsense 23.1.9-amd64
FreeBSD 13.1-RELEASE-p7

Hope this helps! Renaming the php logic solved my issue - works absolutely great now.

@nmiller0113
Copy link

This should address #3408 but ultimately depends on acme.sh cutting a new release with the above pull request included, so I assume would not be able to be merged until then.

Exactly. I'll monitor acme.sh releases and will merge it as soon as possible.

@fraenki just checking if you happened to see the new acme.sh release?

@dmurphynj
Copy link

dmurphynj commented Jun 15, 2023

There is a case-sensitivity issue in LeValidationFactory.php, lines 69-70:

// Convert to PascalCase
$val_name = str_replace(' ', '', ucwords(str_replace(array('-', '_'), ' ', $search_name)));

returns as "DnsGoogledomains" after parsing.

Just to show my I'm-not-a-programmer-and-don't-play-one-on-TV debugging ... I threw a couple error logging lines around the PascalCase code:

    // Convert to PascalCase
        LeUtils::log_error("**dmurphynj: search_name is $search_name");
        $val_name = str_replace(' ', '', ucwords(str_replace(array('-', '_'), ' ', $search_name)));
        LeUtils::log_error("**dmurphynj: val_name is $val_name");

From the ACME Client logfile:

2023-06-15T18:29:25-04:00 | opnsense | AcmeClient: challenge type not supported: dns_googledomains (9f80a3c6-6892-4a91-b3ac-e867df34ad3c)
2023-06-15T18:29:25-04:00 | opnsense | AcmeClient: **dmurphynj: val_name is DnsGoogledomains
2023-06-15T18:29:25-04:00 | opnsense | AcmeClient: **dmurphynj: search_name is dns_googledomains
2023-06-15T18:29:25-04:00 | opnsense | AcmeClient: account is registered: Google Domains

@xArthasx
Copy link
Contributor

Given that all other files follow the same convention of being "DnsX$(lowercaseRest)", @loosecannon93 can you please update the PR? cc @fraenki @nmiller0113

@@ -0,0 +1,49 @@
<?php
Copy link
Member

Choose a reason for hiding this comment

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

As already pointed out by others, this file needs to be renamed to DnsGoogledomains.php.

Choose a reason for hiding this comment

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

@fraenki who would do that? I'm happy to help in whatever way I can :) Thanks!

@fraenki
Copy link
Member

fraenki commented Jun 27, 2023

I'll merge this when the requested change was implemented. acme.sh version 3.0.6 added support for Google Domains.

@fraenki fraenki removed the upstream Third party issue label Jun 27, 2023
@nmiller0113
Copy link

@fraenki is there anything I can do to help move this along? I feel like we've been stuck. I'm happy to help, just not sure what to do. Thanks!

@nmiller0113
Copy link

Or are you waiting on @loosecannon93, @fraenki?

@xArthasx
Copy link
Contributor

xArthasx commented Jul 6, 2023

I think that the issue is that github doesn't allow taking over a PR (AFAIK) so either we have to recreate the entire diff or wait for the owner. At this point I'm hesitant on waiting for the creator to do it, wdyt we should do @fraenki ?

@nmiller0113
Copy link

I think that the issue is that github doesn't allow taking over a PR (AFAIK) so either we have to recreate the entire diff or wait for the owner. At this point I'm hesitant on waiting for the creator to do it, wdyt we should do @fraenki ?

Not sure @xArthasx, only that this has sat dormant for weeks now, and not even @fraenki has commented. Maybe he's just not tracking this right now...who knows? All the pieces are there for this to be completed and for the challenge to be added for opnsense...not really sure what the hold up is.

@xArthasx
Copy link
Contributor

xArthasx commented Jul 8, 2023

Added a new PR for this #3499 as owner is unresponsive cc @nmiller0113

@nmiller0113
Copy link

Added a new PR for this #3499 as owner is unresponsive cc @nmiller0113

Awesome, thanks!

@xArthasx
Copy link
Contributor

@fraenki can you help us move #3499 forward?

@fraenki
Copy link
Member

fraenki commented Jul 11, 2023

PR #3499 was merged

@fraenki fraenki closed this Jul 11, 2023
@opnsense opnsense locked as resolved and limited conversation to collaborators Jul 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature Adding new functionality
Development

Successfully merging this pull request may close these issues.

security/acme-client: Add support for Google Domains DNS API
5 participants