IP identifiers

[Maximebb]

I was going through the code and saw that the AcmeProtocolClient's CreateOrderAsync does not allow the passing of generic identifiers, instead only explicitly supporting dns names. Exposing a list of Identifier could allow the client more flexibility, as in implementing draft specs for IP identifiers: https://tools.ietf.org/html/draft-ietf-acme-ip-05.

Would it be easily implementable in the project's current state? Do it just require a change in method signature/overload?

[ebekker]

It would be pretty easy to support this. Currently the CreateOrderAsync method takes an enumeration of strings and assumes that they're DNS Identifiers, by converting those strings into strongly-typed Identifier instances. To allow more generalized behavior, I would recommend to add a more general overload that takes an enumeration of Identifier directly, and then re-implement the existing method in terms of this more generic variant.

For example, add

        public async Task<OrderDetails> CreateOrderAsync(
            IEnumerable<Identifier> identifiers,
            DateTime? notBefore = null,
            DateTime? notAfter = null,
            CancellationToken cancel = default(CancellationToken))
        {
            // ... slightly altered (simplified) variation of current method goes here...
        }

and then re-implement

        public async Task<OrderDetails> CreateOrderAsync(
            IEnumerable<string> dnsIdentifiers,
            DateTime? notBefore = null,
            DateTime? notAfter = null,
            CancellationToken cancel = default(CancellationToken)) => CreateOrderAsync(
                dnsIdentifiers.Select(x => new Identifier { Type = "dns", Value = x }),
                notBefore, notAfter, cancel);

So it would be pretty easy. Feel free to submit a PR! 😉

[Maximebb]

Great! That's what I thought would be necessary. I will open a PR soon with the changes.

Thanks!

[Maximebb]

Before closing, I have a question:

I will test with the early access nuget feed, but do you know when the next official release will be?

[ebekker]

Whoops! That was actually an oversight, since the testing is coming back clean (at least for Linux), I'm satisfied everything is good to go, so I meant to promote it before, so the library is now updated and published to nuget.

[Maximebb]

I tried the latest version but it didn't contain the merge from my PR. I couldn't figure out from which build it came from though so I don't if it was supposed to contain it.

[Maximebb]

@ebekker not sure if it notifies you if the issue is closed

[ebekker]

Yep, looking into it...

[ebekker]

Not sure what happened, somehow an older version of the code was built and promoted, but I just did a rebuild of the latest and everything looks good now.