Allows specifying external account binding credentials in Caddyfile

[chrisortman]

No description provided.

[CLAassistant]

All committers have signed the CLA.

[francislavoie]

Hmm. I think we want it to look more like this:

acme_ca <url> {
    key_id <value>
    hmac <value>
}

This means modifying the acme_ca parse function to handle a block (with nextBlock) with some sub-options.

[chrisortman]

I initially didn't go that route because I think that means the acme_ca global option will no longer store a string and will have to store a struct? I wasn't sure if external modules might depend on being able to find global options?

I think I could make the parse work this way and but store multiple global options values, but that will break the symmetry of the way the method is right now

[francislavoie]

Well, not necessarily - the Caddyfile config and internal Go structs don't need to match 1:1.

I also don't like the current naming of the keys in the config, they're very opaque and not user-friendly. From a user's perspective, what the heck is "eab"? What's "kid"? Why is there a child in my config? 😂

Having the keys be nested within acme_ca, it's easier to understand what they're for without having a prefix on them, and you can use more user-friendly names for the options.

Take a look at parseOptOnDemand, that's the parsing approach you should take here. Make a new function parseAcmeCa. It should take one argument (the URL) and optionally, a block with some sub-options.

[chrisortman]

I matched the key names to the certbot arguments with the assumption that people will probably have started with that

[francislavoie]

Yeah we don't really are about what certbot does frankly 😛

We can take the opportunity to provide friendlier UX with the Caddyfile.

[chrisortman]

Will changing the value stored in the global_options[acme_ca] be considered a breaking change? Are modules allowed to pull values from that map?

[mholt]

I agree with @francislavoie on this one -- I prefer the block syntax, to keep everything together.

I will have a chance to review in more detail this in a bit!

[francislavoie]

Modules could probably pull from that map, but I can't envision any usecase where they really would. It's not important to external modules. This is config for the tls app in Caddy, and it's only a Caddyfile-specific thing.

None of the modules in this list have any business doing something like that, so I'm not concerned.

[chrisortman]

What do I do to fix the lint check? go fmt didn't change any files when I ran it ..

My mistake, I assumed go fmt was recursive on it's own.

[francislavoie]

Should leave this in, actually. That should continue to work.

[chrisortman]

I can put it back, I removed it so that I had something testing when the option wasn't specified at all,

[chrisortman]

I suspect this is why the IBM check fails? #3493

[francislavoie]

Yeah, don't mind that for the time being. We're fixing it 😄

[mholt]

@chrisortman Thanks for this -- I think I have a slightly better idea than what I proposed before. I'lll submit a new commit to your branch (building on what you've implemented here) in a little while and see what you think!

[mholt]

@chrisortman Okay, see what you think of this. Revised syntax:

{
    acme_eab {
       key_id <key>
       hmac   <hmac>
    }
}

It's separate from acme_ca but still grouped together.

Please try it out and let me know what you think!

[francislavoie]

Does it need to be eab? That's so opaque. Could it just be acme_account? Docs can elaborate.

[mholt]

I thought about that. That's the technical, unambiguous term, though. And it's actually not the ACME account, it's specifically an external account binding -- not sure there's a way around that. The ACME account is based on your user key pair, which is managed by CertMagic according to your email parameter.

[chrisortman]

I like this better, makes it so there doesn't have to be the extra ACMECaConfig struct too

[mholt]

@chrisortman Great! If it works for you, we'll merge this in.

[chrisortman]

Yes works for me.

[chrisortman]

On a side note, I did not expect that you would be able to push directly to my fork, has github always done that? Is it limited to the original owner or all collaborators?

[mholt]

@chrisortman I believe it has always done that. There's a checkbox you can check/uncheck when making the pull request to allow commits from maintainers. I think we can only push to the branch you propose.

[chrisortman]

Thanks @mholt I have never noticed that before.