Android: order is valid however certificate chain is not correctly downloaded about acme4j HOT 19 CLOSED

Thank you for reporting back! I have just published the fix in acme4j v2.13. It's available in the release section, and should be available on Maven Central in the next couple of hours.

from acme4j.

@shred I can confirm the problem is solved on my end! ACME4J is functional on Android.

from acme4j.

Hi Niels!

What CA are you connecting to?

When you paste the URL in a browser, the certificate is fetched via GET request. However according to the RFC 8555 certificates are supposed to be fetched via a "POST-as-GET" request, which is essentially a signed POST request with an empty body. This is what acme4j does.

It sounds to me like the CA does not support POST-as-GET requests for fetching certificates. However there should still be a AcmeLazyLoadingException runtime exception thrown by acme4j.

If you enable DEBUG logging on acme4j, you can see the full communication between server and client. Maybe this will give a hint about what is going wrong. (If you use the simple slf4j logger, you can enable debug logging with e.g. -Dorg.slf4j.simpleLogger.log.org.shredzone.acme4j=debug).

from acme4j.

Hi @shred,

Thanks for the quick response, its connecting to a modified Boulder CA. I'll try and check if that supports "POST-as-GET". But there is no AcmeLazyLoadingException. The download just succeeds and certChain = new ArrayList<>(conn.readCertificates()); results in an empty list.

I'll try and figure out how to enable debug logging for Acme4j, I'm using it in an Android application.

from acme4j.

If it's a Boulder CA, I am confident that POST-as-GET should work there. That doesn't seem to be the problem.

You are the first one I know who is using acme4j on Android. I have never really tested the code against the Android API, so maybe it's related to that.

I had to introduce the TrimmingInputStream class because IBM's JVM was unable to read the certificate chain otherwise. Maybe it causes a problem with Android's CertificateFactory.generateCertificates() implementation now? At least, the Android API docs state that the result of generateCertificates() is an empty list if "no certificates are present".

Can you compile acme4j locally? Then could you just remove the TrimmingInputStream from DefaultConnection.readCertificate() for a test? I can also provide you a jar file with that change, for testing purposes.

To get the debug output to logcat, you might need something like org.slf4j:slf4j-android:1.7.36.

from acme4j.

There's a first for everything!

If you could provide a jar that would be great, I will test it asap. Might be the Android implementation indeed.

from acme4j.

Here it is: acme4j-client-2.13-SNAPSHOT.zip

GitHub does not support the upload of jar files, so I had to rename it. Just rename it back from .zip to .jar.

I have removed the TrimmingInputStream, and also added some more debug output in case it didn't help. When you invoke getCertificate(), you should find something like this in logcat now:

DEBUG org.shredzone.acme4j.connector.DefaultConnection - ** reading certificate, content type = application/pem-certificate-chain
DEBUG org.shredzone.acme4j.connector.DefaultConnection - ** content type accepted, opening stream
DEBUG org.shredzone.acme4j.connector.DefaultConnection - ** Reading certificates
DEBUG org.shredzone.acme4j.connector.DefaultConnection - **   Found certificate CN=example.com
DEBUG org.shredzone.acme4j.connector.DefaultConnection - **   Found certificate CN=Pebble Intermediate CA 645fc5
DEBUG org.shredzone.acme4j.connector.DefaultConnection - ** Reading completed, found 2 certificates in the chain

from acme4j.

Thanks for the .jar!

I've included your changes and am successfully able to debug the new code as well. It did not fix the issue though.

List<X509Certificate> result = (List)cf.generateCertificates(in).stream().map((c) -> {
       return (X509Certificate)c;
}).peek((c) -> {
        LOG.debug("**   Found certificate " + c.getSubjectDN().getName());
}).collect(Collectors.toList());

Still returns an empty list unfortunately so the LOG with CN is not printed. What is weird is that if I change generateCertificates(in) to generateCertificate(in) it actually successfully loads the (presumably first) certificate in a file of 2 certificates which makes me think it's a parsing issue.

The file has no extension and has the following format:

-----BEGIN CERTIFICATE-----
...
-----END CERTIFICATE-----

-----BEGIN CERTIFICATE-----
...
-----END CERTIFICATE-----

It has newline breaks after each certificate, maybe that could be the issue. Do you know if this format is up to spec with the RFC? Looking at the specs I see their example not using line breaks. And I did find this:

https://stackoverflow.com/questions/48083276/reading-data-in-a-pem-certificate-chain/50029488

However that seems to be a slightly different problem because for them generateCertificates(in) returns the first certificate, not an empty list.

In any case: thanks for the help so far, I'll try and see if I can think of something else that could be the problem.

from acme4j.

The problem with the empty line sounds familiar, see issue #60. This is why I added the TrimmingInputStream first place. Maybe I made it worse by removing it?

This is a version with the same debug output, but the TrimmingInputStream back into place: acme4j-client-2.13-SNAPSHOT.zip Does it change anything?

I will also do some checks with Android and generateCertificates() here. However I first need to set up an Android IDE again (my last Android attempts are some years ago now), and I don't have much time for that until next week.

Please keep me updated. If I can assist you, just let me know.

from acme4j.

Unfortunately that doesn't change anything. It's definitely related to some kind of whitespace, \n or \r related since I can reproduce the problem using local data. Apologies for using Kotlin here.

val certs = """
        -----BEGIN CERTIFICATE-----
        .... (omitted)
        -----END CERTIFICATE-----
        -----BEGIN CERTIFICATE-----
        ... (omitted)
        -----END CERTIFICATE-----
    """.trimIndent()

val certs = certificateFactory.generateCertificates(cert.byteInputStream())

results in 2 certificates.

// Notice the empty lines
val certs = """
        -----BEGIN CERTIFICATE-----
        .... (omitted)
        -----END CERTIFICATE-----

        -----BEGIN CERTIFICATE-----
        ... (omitted)
        -----END CERTIFICATE-----

    """.trimIdent()

val certs = certificateFactory.generateCertificates(cert.byteInputStream())

Results in one certificate.

// Notice the empty lines and the omission of trimIdent()
val certs = """
        -----BEGIN CERTIFICATE-----
        .... (omitted)
        -----END CERTIFICATE-----

        -----BEGIN CERTIFICATE-----
        ... (omitted)
        -----END CERTIFICATE-----

    """

val certs = certificateFactory.generateCertificates(cert.byteInputStream())

Results in no certificates.

I have tried wrapping the original inputStream in another which removes empty lines but that seems to be not enough. I'll keep you posted.

from acme4j.

TrimmingInputStream normalizes all sequences of at least one \r or \n into a single \n (similar to regex replaceAll("[\\r\\n]+","\n")). The result should be like in your first example.

So either my TrimmingInputStream implementation is buggy, or Android does not like pure \n as line separator.

from acme4j.

Might be that the Android implementation (which I believe is using https://github.com/google/conscrypt) does not like a \n after the last certificate.

val cert = """
        -----BEGIN CERTIFICATE-----
        .. (omitted)
        -----END CERTIFICATE-----
        -----BEGIN CERTIFICATE-----
        .. (omitted)
        -----END CERTIFICATE-----
    """.trimStart() // no trimIndent()

Also results in no certificates, so the

/**
 * Detects a common minimal indent of all the input lines, removes it from every line and also removes the first and the last
 * lines if they are blank (notice difference blank vs empty).
 *
 * Note that blank lines do not affect the detected indent level.
 *
 * In case if there are non-blank lines with no leading whitespace characters (no indent at all) then the
 * common indent is 0, and therefore this function doesn't change the indentation.
 *
 * Doesn't preserve the original line endings.
 */

docs of trimIdent() might have clue since it also removes the first and last line if it is empty, so I think also when there is a single \n at the start of the sequence and the end of the last certificate in the sequence. Which your TrimmingInputStream would keep?

Anyway the whole thing is kind of suspect since according to RFC 7468 clearly states:

Data before the encapsulation boundaries are
permitted, and parsers MUST NOT malfunction when processing such
data. Furthermore, parsers SHOULD ignore whitespace and other non-
base64 characters and MUST handle different newline conventions.

But as you said IBM's JVM did not do that correctly either.

from acme4j.

Yes, TrimmingInputStream is removing all heading line breaks, but is keeping a single trailing \n.

In this test version, all heading and trailing line breaks are trimmed: acme4j-client-2.13-SNAPSHOT.zip

The line breaks are now at the bare minimum. Does it work? 😃

from acme4j.

Wish I had better news.. zero certificates. Must be something else then.

from acme4j.

I guess I have found the problem. Conscrypt's generateCertiifcates() invokes InputStream.available() and requires that the stream has at least one non-blocking byte available. IMO this is a bug in Conscrypt, expecially because generateCertificate() does not have that restriction. I will open an issue there.

For acme4j I have added a workaround that uses a BufferedInputStream and pre-fetches one byte, so available() won't return 0. It works on my local Android test. What about you?

acme4j-client-2.13-SNAPSHOT.zip

from acme4j.

@shred sorry for the delay, coincidentally we have some issues with our CA which makes it not possible for me to test right now. If it is fixed I will delete this comment and post my findings :) Thanks for diving in this issue by the way!

from acme4j.

Thank you for reporting this issue, @NielsMasdorp! Now I know that there is a use case for acme4j on Android, and that it is (hopefully) also working there. 😃

As soon as you confirm that the bug is fixed for you, I will push a new release to Maven Central.

from acme4j.

I unintentionally closed this issue by pushing a fix to the main branch.

I am still interested in your feedback! If the issue isn't resolved for you, please reopen. I will release a new version as soon as you confirm it's fixed for you,.

from acme4j.

No worries! Rest assured I will update you. Kind of ashamed the backend team is taking this long, but I should be able to test on monday. Have a nice weekend!

from acme4j.