gmpassos / shelf_letsencrypt

Let's Encrypt support for the shelf package (free and automatic HTTPS certificate support).
Apache License 2.0
8 stars 3 forks source link

doAcmeChallenge ignores alternate port no. #8

Closed bsutton closed 8 months ago

bsutton commented 9 months ago

For my dev environment I start the server on port 8080.

The call to doACMEChallenge is failing is it builds a URL without a port number with the result that it tries to connect on port 80 and fails.

bsutton commented 9 months ago

This seems to be happening in acme_client getHttpDcvData

  HttpDcvData getHttpDcvData() {
    var keyAuthorization = getKeyAuthorizationForChallenge(VALIDATION_HTTP);
    var token = keyAuthorization!.split('.').elementAt(0);
    return HttpDcvData(
      '${identifier!.value}/.well-known/acme-challenge/$token',
      keyAuthorization,
      getChallengeByType(VALIDATION_HTTP),
    );
  }

the identifier.value has the domain name but no port no.

bsutton commented 9 months ago

We see the following message logged before the call is made (and fails)

Self test challenge... {type: HTTP, fileName: squarephone.biz/.well-known/acme-challenge/ksPAAcFzl5CSUQNXwymuoLcZ0NEpsGTHK00Y5AFQXRs, fileContent: ksPAAcFzl5CSUQNXwymuoLcZ0NEpsGTHK00Y5AFQXRs.wZQeeUT1GoiIsb-N7YRgyTccyL7GBnF_QfeYc6w2rVc, challenge: Instance of 'Challenge'

bsutton commented 9 months ago

So I think the problem is here:

Future<bool> _selfChallengeTest(
      AcmeClient client, HttpDcvData challengeData) async {
    var url = challengeData.fileName;
    if (!url.startsWith('http:') && !url.startsWith('https:')) {
      final idx = url.indexOf(':/');
      if (idx >= 0) {
        final schema = url.substring(0, idx);
        var rest = url.substring(idx);
        rest = rest.replaceFirst(RegExp('^/+'), '');
        url = '$schema://${rest.replaceAll('//', '/')}';
      } else {
        final rest = url.replaceFirst(RegExp('^/+'), '');
        url = 'http://${rest.replaceAll('//', '/')}';
      }
    }

We are pre-pending HTTP:// but we ignore the need to add in the port.

bsutton commented 9 months ago

I will see if I can fix it.

bsutton commented 9 months ago

I have a fix for it, it does mean passing the httpPort around quite a bit.

gmpassos commented 9 months ago

The issue is only on shelf_letsencrypt?

bsutton commented 9 months ago

The bug was on shelf_letsenctypt, I've fixed it.

On Sat, 9 Dec 2023, 6:38 am Graciliano Monteiro Passos, < @.***> wrote:

The issue is only on shelf_letsencrypt?

— Reply to this email directly, view it on GitHub https://github.com/gmpassos/shelf_letsencrypt/issues/8#issuecomment-1847726534, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG32OAM3EHEWBCNIPNAYETYINUD5AVCNFSM6AAAAABAMPGHTGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBXG4ZDMNJTGQ . You are receiving this because you authored the thread.Message ID: @.***>

gmpassos commented 9 months ago

Nice 👍

Since your PR includes a lot of improvements, I will take a few days to review all of the code and make adjustments on my machine.

Additionally, considering your significant contribution, please include your name and GitHub page in the "Author" section.

bsutton commented 9 months ago

Thank you, I will do that.

Fyi: in raising a pr to give the acme_client bug.

On Sat, 9 Dec 2023, 8:00 am Graciliano Monteiro Passos, < @.***> wrote:

Nice 👍

Since your PR includes a lot of improvements, I will take a few days to review all of the code and make adjustments on my machine.

Additionally, considering your significant contribution, please include your name and GitHub page in the "Author" section.

— Reply to this email directly, view it on GitHub https://github.com/gmpassos/shelf_letsencrypt/issues/8#issuecomment-1847837277, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG32OBTQ5NB7QJKJDHXXALYIN5WPAVCNFSM6AAAAABAMPGHTGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBXHAZTOMRXG4 . You are receiving this because you authored the thread.Message ID: @.***>

gmpassos commented 9 months ago

...Also, I will need to test it in the staging environment of some internal projects that are using shelf_letsencrypt.

bsutton commented 9 months ago

Sound good, key me know if I've broken anything:)

On Sat, 9 Dec 2023, 8:02 am Graciliano Monteiro Passos, < @.***> wrote:

...Also, I will need to test it in the staging environment of some internal projects that are using shelf_letsencrypt.

— Reply to this email directly, view it on GitHub https://github.com/gmpassos/shelf_letsencrypt/issues/8#issuecomment-1847839698, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG32OCVHQPWAHURJ5OH643YIN577AVCNFSM6AAAAABAMPGHTGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBXHAZTSNRZHA . You are receiving this because you authored the thread.Message ID: @.***>

bsutton commented 9 months ago

So this looks to be a bigger issue. Anywhere we do a call back to the local server the code is broken because it ignores the current port.

Worry the current api the porr isn't known until the users starts the server but we have a number methods that are valid independently of starting the server.

It kids to be like we need to move the port and bind address to the letsencrpt constructor.

This would be a breaking change but I think it's the right way to do it.

Thoughts?

gmpassos commented 9 months ago

The point is that LetsEncrypt should be able to handle multiple domains in the same address:port.

The custom port is only needed for the HTTP challenge? If it's true, it knows the listening port of the HTTP server that it started. (should be inserted in the HTTP URL only if it's different than 80).

bsutton commented 9 months ago

The point is that LetsEncrypt should be able to handle multiple domains in the same address:port. I don't believe that I've made any changes to inhibit this. Have you raised this due to a concern or problem?

The custom port is only needed for the HTTP challenge?

I had to make changes in two spots:

should be inserted in the HTTP URL only if it's different than 80). I've been a little lazy by always inserting the port. It's essentially invisible to the user and valid so I don't really see the issue. If you want I can make it conditional?

gmpassos commented 9 months ago

Make the changes. I don't think they will be backward incompatible.

bsutton commented 9 months ago

I think I've now made all the changes I'm going to, so go ahead and merge if you are happy with everything.

gmpassos commented 8 months ago

Do you want to make any new adjustment or wait some dependency fix/patch?

bsutton commented 8 months ago

I'm all done.

On Sat, Dec 16, 2023 at 7:33 AM Graciliano Monteiro Passos < @.***> wrote:

Do you want to make any new adjustment or wait some dependency fix/patch?

— Reply to this email directly, view it on GitHub https://github.com/gmpassos/shelf_letsencrypt/issues/8#issuecomment-1858453413, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG32OGJIEHJPI7BO4QLU2TYJSXYLAVCNFSM6AAAAABAMPGHTGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNJYGQ2TGNBRGM . You are receiving this because you authored the thread.Message ID: @.***>

bsutton commented 8 months ago

Fixed via: https://github.com/gmpassos/shelf_letsencrypt/pull/6