Makes DNS validation selectable by domain: each SAN (additional domain) can use different validation

[baquilla]

Sometimes I need to validate a domain (SAN) with DNS while keeping the rest with HTTP validation. Although in some cases I could switch all validation to DNS, this makes the process slower. So, I've modified the functionality of VALIDATE_VIA_DNS to also accept a list of domains, and only these will be validated with DNS.

[killerbees19]

Just use these variable(s) in your domain cfg and override the (global) default config? Or use the source statement to include other config files. There's no need to define everything globally...

[baquilla]

Just use these variable(s) in your domain cfg and override the (global) default config? Or use the source statement to include other config files. There's no need to define everything globally...

Can you give me an example? I could not figure out how to do that, I already use dom cfg. Take into account that this feature is not for each domain (certificate) it's for each SAN included in a domain (certificate).

For example, setting this in domain cfg:
DOMAIN=domain1.com
SANS=domain2.com domain3.com domain4.com
VALIDATE_VIA_DNS=domain2.dom

All the domains do validation with http, however domain2.com cannot validate via http and needs to validate via dns. The others cannot validate via dns., must continue using http.

Using this PR and VALIDATE_VIA_DNS=domain2.dom I can properly get that behavior. Is currently there another way to get this behavior?

Thanks

[killerbees19]

Take into account that this feature is not for each domaiin (certificate) it's for each SAN included in a domain (certificate).

My bad, I misunderstood this part. Sorry! 🫣

[tlhackque]

I can see where this would be useful. Thanks for the attempt. It's a good start, but it needs more work.

One obstacle to getting it accepted is the lack of test coverage. (See the test/ subdirectory in git. There's a learning curve. In my case, also a forgetting curve.)

Functionally: there are other references to VALIDATE_VIA_DNS == "true" that would have to change. E.g. in the sanity checks for DNS_{ADD,DEL}_COMMAND, you'd need to use [[ -n "$VALIDATE_VIA_DNS" ]]. (This checks [0].)

There are similar uses when checking for issuing wildcard certificates and for ACLs required for http-01. You have to address every use of VALIDATE_VIA_DNS.

Rather than the regex parser, consider making VALIDATE_VIA_DNS into an array. That's consistent with other configuration variables in getssl, and straightforward to code. e.g.

VALIDATE_VIA_DNS=               # Disabled
VALIDATE_VIA_DNS=('A' 'B' 'C')  # Enabled (only) for A B and C
#VALIDATE_VIA_DNS="true"        # Enabled globally (for all)

# Return true if $dom validates via `dns-01` (otherwise, http-01)
validates_via_dns( ){
  local dom="$1"
  [[ "$VALIDATE _VIA_DNS" == "true" ]] && return 0 # Global enable: Checks [0] if array, value if not
  [[ -z "$VALIDATE _VIA_DNS" ]]        && return 1 # Disabled for all
  for d in "${VALIDATE_VIA_DNS[@]}"; do
    [[ "$d == "$dom" ]]                && return 0 # Enabled for this one
  done
  return 1                             # Selective, but no match for this one
}

This does have a problem if the true TLD is ever issued. But in that case, you'd enter it as 'TRUE' for the domain. Don't lose sleep over that one :-)

[baquilla]

Hi @tlhackque

One obstacle to getting it accepted is the lack of test coverage. (See the test/ subdirectory in git. There's a learning curve. In my case, also a forgetting curve.)

I think the tests would only have to validate the default behavior (VALIDATE_VIA_DNS == "" or "true"). I understand that testing this new feature would be desirable, but the goal is to avoid breaking what it's currently working. If I have time I'll try to review it, however I have already dealt with the tests and they seam to fail randomly, which is demotivating. I tried to run them setting up all the resources required and sometimes works and others not. That behaviour have unmotivated me to work on it. No excuse.

Functionally: there are other references to VALIDATE_VIA_DNS == "true" that would have to change. E.g. in the sanity checks for DNS_{ADD,DEL}_COMMAND, you'd need to use [[ -n "$VALIDATE_VIA_DNS" ]]. (This checks [0].)

Sorry but I couldn't find more references to VALIDATE_VIA_DNS in the code, nor the scripts. I think I have already changed it the sanity checks before calling DNS_{ADD,DEL}_COMMAND: if validate_via_dns; then # using dns-01 challenge

Can you please guide me to the correct place so I can fix it?

There are similar uses when checking for issuing wildcard certificates and for ACLs required for http-01. You have to address every use of VALIDATE_VIA_DNS.

I guess I did. I'm using it for wildcard domains. The regext takes it into account. I couldn't find any miss use of VALIDATE_VIA_DNS.

Rather than the regex parser, consider making VALIDATE_VIA_DNS into an array. That's consistent with other configuration variables in getssl, and straightforward to code. e.g.

Hey man, that sound great but VALIDATE_VIA_DNS is expected to work like SANS because SANS contains the domain list that should be validated in a different way. This feature makes no sense if no SANS are defined. Therefore I have used the same format used for SANS. Additionally, SANS are parsed using regex. No invention from my side.

# Ensure SANS is comma separated by replacing any number of commas or spaces with a single comma
# shellcheck disable=SC2001
SANS=$(echo "$SANS" | sed 's/[, ]\+/,/g')

and later used as it's or splitted into and array depending on each particular requirements.

Makes no sense to me to use a different approach to define domain lists in the same context. Point me to the right path if I'm wrong. I may have misunderstood you proposal.

This does have a problem if the true TLD is ever issued. But in that case, you'd enter it as 'TRUE' for the domain. Don't lose sleep over that one :-)

Um... you're right. The regex is wrong and should validate it properly.: (^[Tt][Rr][Uu][Ee]$|(^|[ ,])${d}($|[ ,])). My fault.

Beyond my "retorts", thank you very much for taking your time to review my PR.

[timkimber]

@baquilla Thanks for making these changes, and @tlhackque thanks so much for reviewing them! I'll merge as soon as I get a chance.

@baquilla regarding the tests:

I have already dealt with the tests and they seam to fail randomly, which is demotivating. I tried to run them setting up all the resources required and sometimes works and others not. That behaviour have unmotivated me to work on it. No excuse.

I've tried to make them less-flakey, the acme-dns ones are the worst as they depend on the vagaries of global DNS propagation. The others should work unless you hit one of the "not reproducible easily" bugs (from memory if one of the values returned from the pebble server begins with a "-" then something fails.

If you can suggest anyway to make them more reliable, or tell me which caused you problems, then I'll have another attempt at making them more robust.

[tlhackque]

Hi @tlhackque

One obstacle to getting it accepted is the lack of test coverage. (See the test/ subdirectory in git. There's a learning curve. In my case, also a forgetting curve.)

I think the tests would only have to validate the default behavior (VALIDATE_VIA_DNS == "" or "true"). I understand that testing this new feature would be desirable, but the goal is to avoid breaking what it's currently working. If I have time I'll try to review it, however I have already dealt with the tests and they seam to fail randomly, which is demotivating. I tried to run them setting up all the resources required and sometimes works and others not. That behaviour have unmotivated me to work on it. No excuse.

Yes, the tests are painful. But important. If a feature is important enough to add, it's important enough to test. Especially this one, which modifies existing behavior. Things break otherwise - CNAME redirection of challenges is a current example.

Functionally: there are other references to VALIDATE_VIA_DNS == "true" that would have to change. E.g. in the sanity checks for DNS_{ADD,DEL}_COMMAND, you'd need to use [[ -n "$VALIDATE_VIA_DNS" ]]. (This checks [0].)

Sorry but I couldn't find more references to VALIDATE_VIA_DNS in the code, nor the scripts. I think I have already changed it the sanity checks before calling DNS_{ADD,DEL}_COMMAND: if validate_via_dns; then # using dns-01 challenge

Can you please guide me to the correct place so I can fix it?

Here my point is is that if we adopt the change to an array, all the current code references need to either check [0] or ${[*]}.

That's a small price to pay for (mostly) getting rid or the regex parsing.

There are similar uses when checking for issuing wildcard certificates and for ACLs required for http-01. You have to address every use of VALIDATE_VIA_DNS.

I guess I did. I'm using it for wildcard domains. The regext takes it into account. I couldn't find any miss use of VALIDATE_VIA_DNS.

Rather than the regex parser, consider making VALIDATE_VIA_DNS into an array. That's consistent with other configuration variables in getssl, and straightforward to code. e.g.

Hey man, that sound great but VALIDATE_VIA_DNS is expected to work like SANS because SANS contains the domain list that should be validated in a different way. This feature makes no sense if no SANS are defined. Therefore I have used the same format used for SANS. Additionally, SANS are parsed using regex. No invention from my side.

# Ensure SANS is comma separated by replacing any number of commas or spaces with a single comma
# shellcheck disable=SC2001
SANS=$(echo "$SANS" | sed 's/[, ]\+/,/g')

Actually, I was thinking of converting SANS into an array (with a compatibility shim), so replicating SANS would only create more compatibility code. The ACL variable is an array. They are less error-prone to enter - each item can be on a separate line. The delimiter is white space - not comma which is confused with period and often people add a space. You don't have to parse them into arrays - as you note, that's what happens here. It's easy to determine how many elements are in an array - no parsing required. And parsing is more expensive than an array reference. So it's better to start as an array...
For compatibility with old config files, one does a single test to see if there's a ',', and only in that case split into an array. Well outside of the actual processing code, which gets simpler.

and later used as it's or splitted into and array depending on each particular requirements.

Makes no sense to me to use a different approach to define domain lists in the same context. Point me to the right path if I'm wrong. I may have misunderstood you proposal.

This does have a problem if the true TLD is ever issued. But in that case, you'd enter it as 'TRUE' for the domain. Don't lose sleep over that one :-)

Um... you're right. The regex is wrong and should validate it properly.: (^[Tt][Rr][Uu][Ee]$|(^|[ ,])${d}($|[ ,])). My fault.
No. You don't want a case-insensitive comparison here. getssl always compares lower case true and false.
My point was that we have to handle the case of a domain named "true.". Suppose someone wanted to issue a certificate for
SANS="true,true,www.mail.true". You could do that with "com" or "fr" today. You can't do that with your change.

This is unlikely, but people do strange things. My observation was that since DNS is case-insensitive, the user would handle this by putting "TRUE" in VALIDATE_VIA_DNS instead of "true". Your suggested revision would break that.

Beyond my "retorts", thank you very much for taking your time to review my PR.

[tlhackque]

@baquilla Thanks for making these changes, and @tlhackque thanks so much for reviewing them! I'll merge as soon as I get a chance.

@baquilla regarding the tests:

I have already dealt with the tests and they seam to fail randomly, which is demotivating. I tried to run them setting up all the resources required and sometimes works and others not. That behaviour have unmotivated me to work on it. No excuse.

I've tried to make them less-flakey, the acme-dns ones are the worst as they depend on the vagaries of global DNS propagation. The others should work unless you hit one of the "not reproducible easily" bugs (from memory if one of the values returned from the pebble server begins with a "-" then something fails.

If you can suggest anyway to make them more reliable, or tell me which caused you problems, then I'll have another attempt at making them more robust.

@timkimber I think this needs more thought before merging. See my second round of comments below.

[tlhackque]

I've started a new branch to deal with this. tlhackque/selective-validation. No commits yet, but there's work in progress.

@baquilla: if you can work on the tests, it would help.