jchambers / pushy

A Java library for sending APNs (iOS/macOS/Safari) push notifications
https://pushy-apns.org/
MIT License
1.81k stars 450 forks source link

Breaking change in 0.15.1 regarding ApnsClient, ApnsClientBuilder #951

Open lauredogit opened 2 years ago

lauredogit commented 2 years ago

Hi,

We tried upgrading from 0.15.0 to 0.15.1 and we are observing a breaking change in the API of the ApnsClientBuilder / ApnsClient.

We extended these classes to be able to configure our own SSLContext, which is allowing us to use a dynamic trust manager or the cipher suites of our own choice.

In 0.15.1, the way to pass an SSLContext to the ApnsClient is via the new ApnsClientConfiguration which is package-private.

Would it be possible to make ApnsClientConfiguration public instead so that our code can keep working?

Thanks.

jchambers commented 2 years ago

Sorry to hear that this has been a problem for you. Slipping in a breaking change was a mistake, and was not intentional; I should have caught it before shipping.

That said, I'm hesitant to make ApnsClientConfiguration public. Can you tell me more about your use case? Can you tell me more about the need for a "dynamic trust manager" or why the default cipher suite isn't appropriate for you?

lauredogit commented 2 years ago

So far, we were able to extend the ApnsClient to modify its behaviour.

Now that the constructor only accepts a package-private ApnsClientConfiguration instance, it's no longer the case.

We have IT security policies where some cipher suites are stongly recommended for example.

Regarding the dynamic trust manager, we can update trusted certificates for all our services from a central service, so for us, statically adding trusted certificates is not enough.

What we have acutally done is that we extended the ApnsClientBuilder to add the capability to configure a trust manager directly and then we simply modified the build() method to take this into account when constructing the SSLContext.

            } else if (this.trustedServerCertificates != null) {
                sslContextBuilder.trustManager(this.trustedServerCertificates);
            /*
              Customization for our project.
             */
            } else if (this.trustManager != null) {
                sslContextBuilder.trustManager(this.trustManager);
            }

            sslContext = sslContextBuilder.build();

Letting users set their own trust manager would equally solve this for us.

   /**
     * Customization for our project.
     */
    public ExtendedApnsClientBuilder setTrustManager(final TrustManager trustManager) {
        this.trustedServerCertificatePemFile = null;
        this.trustedServerCertificateInputStream = null;
        this.trustedServerCertificates = null;
        this.trustManager = trustManager;

        return this;
    }
jchambers commented 2 years ago

Okay; thanks for the added context. Let me think on this a bit. I think this is fundamentally incompatible with some of what we're trying to do to get multi-credential clients in play, but maybe not.

I hesitate to expose TrustManager in the general case because (seemingly) the most common advice for "I am having certificate problems in Java" is "copy/paste this magic spell that creates a TrustManager that trusts everything."

Like I said, though: let me think about it and see what I can come up with.

lauredogit commented 2 years ago

We do have a magic TrustManager, which actually reloads trusted certificates from a central service, creating an in-memory KeyStore from them and instantiating a matching TrustManager every time there is a change.

Here's a generic example:

import java.io.IOException;
import java.net.Socket;
import java.security.KeyStore;
import java.security.KeyStoreException;
import java.security.NoSuchAlgorithmException;
import java.security.cert.CertificateException;
import java.security.cert.X509Certificate;
import java.util.concurrent.atomic.AtomicReference;
import java.util.function.Supplier;

import javax.net.ssl.SSLEngine;
import javax.net.ssl.TrustManagerFactory;
import javax.net.ssl.X509ExtendedTrustManager;

import com.google.common.collect.ImmutableSet;

import org.jetbrains.annotations.NotNull;

/**
 * Dynamic {@link X509ExtendedTrustManager} which permits reloading trusted certificates provided by a cache of
 * certificates.
 * <p>
 * Staleness is determined by reference equality so the cache has to provide the same cached instance until it becomes
 * stale.
 */
public final class DynamicX509ExtendedTrustManager extends X509ExtendedTrustManager {

    static final class AtomicState {

        @NotNull
        private final ImmutableSet<X509Certificate> trustedCertificates;

        @NotNull
        private final X509ExtendedTrustManager trustManager;

        AtomicState(@NotNull ImmutableSet<X509Certificate> trustedCertificates) {
            this.trustedCertificates = trustedCertificates;
            trustManager = newTrustManager(this.trustedCertificates);
        }

        @SuppressWarnings("ReferenceEquality")
        boolean isStale(@NotNull ImmutableSet<X509Certificate> currentCertificates) {
            return trustedCertificates != currentCertificates;
        }

        @NotNull
        X509ExtendedTrustManager getTrustManager() {
            return trustManager;
        }
    }

    @NotNull
    static TrustManagerFactory newTrustManagerFactory(@NotNull Iterable<X509Certificate> trustedCertificates) {
        try {
            KeyStore keyStore = KeyStore.getInstance(KeyStore.getDefaultType());
            keyStore.load(null, null);

            int i = 0;
            for (final X509Certificate trustedCertificate : trustedCertificates) {
                String alias = Integer.toString(++i);
                keyStore.setCertificateEntry(alias, trustedCertificate);
            }

            // Set up trust manager factory to use our key store.
            TrustManagerFactory trustManagerFactory =
                    TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());

            trustManagerFactory.init(keyStore);

            return trustManagerFactory;
        } catch (KeyStoreException | IOException | NoSuchAlgorithmException | CertificateException ex) {
            throw new IllegalStateException("Unable to create new TrustManagerFactory: " + ex, ex);
        }
    }

    @NotNull
    static X509ExtendedTrustManager newTrustManager(@NotNull Iterable<X509Certificate> trustedCertificates) {
        TrustManagerFactory trustManagerFactory = newTrustManagerFactory(trustedCertificates);
        return (X509ExtendedTrustManager) trustManagerFactory.getTrustManagers()[0];
    }

    @NotNull
    private final Supplier<ImmutableSet<X509Certificate>> cachedCertificateSupplier;

    @NotNull
    private final AtomicReference<AtomicState> stateRef = new AtomicReference<>(new AtomicState(ImmutableSet.of()));

    public DynamicX509ExtendedTrustManager(@NotNull Supplier<ImmutableSet<X509Certificate>> cachedCertificateSupplier) {
        this.cachedCertificateSupplier = cachedCertificateSupplier;
    }

    @NotNull
    private X509ExtendedTrustManager getTrustManager() {
        while (true) {
            AtomicState currentState = stateRef.get();
            ImmutableSet<X509Certificate> currentCertificates = cachedCertificateSupplier.get();
            if (!currentState.isStale(currentCertificates)) {
                return currentState.getTrustManager();
            }
            AtomicState updatedState = new AtomicState(currentCertificates);
            if (stateRef.compareAndSet(currentState, updatedState)) {
                return updatedState.getTrustManager();
            }
        }
    }

    @Override
    public void checkClientTrusted(X509Certificate[] x509Certificates, String authType, Socket socket)
            throws CertificateException {
        getTrustManager().checkClientTrusted(x509Certificates, authType, socket);
    }

    @Override
    public void checkServerTrusted(X509Certificate[] x509Certificates, String authType, Socket socket)
            throws CertificateException {
        getTrustManager().checkServerTrusted(x509Certificates, authType, socket);
    }

    @Override
    public void checkClientTrusted(X509Certificate[] x509Certificates, String authType, SSLEngine sslEngine)
            throws CertificateException {
        getTrustManager().checkClientTrusted(x509Certificates, authType, sslEngine);
    }

    @Override
    public void checkServerTrusted(X509Certificate[] x509Certificates, String authType, SSLEngine sslEngine)
            throws CertificateException {
        getTrustManager().checkServerTrusted(x509Certificates, authType, sslEngine);
    }

    @Override
    public void checkClientTrusted(X509Certificate[] x509Certificates, String authType) throws CertificateException {
        getTrustManager().checkClientTrusted(x509Certificates, authType);
    }

    @Override
    public void checkServerTrusted(X509Certificate[] x509Certificates, String authType) throws CertificateException {
        getTrustManager().checkServerTrusted(x509Certificates, authType);
    }

    @Override
    public X509Certificate[] getAcceptedIssuers() {
        return getTrustManager().getAcceptedIssuers();
    }
}