From: Robert Varga Date: Mon, 29 Jan 2024 23:15:42 +0000 (+0100) Subject: Move data processing to update thread X-Git-Tag: v7.0.0~77 X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?a=commitdiff_plain;h=abdc43539a3756f71e687f061d571cf965e58f0d;p=netconf.git Move data processing to update thread We are interpreting datastore data and applying to Java native constructs. Rather than doing that each time we are asked for a SslHandler, move the processing to when the data changes. This also makes things a tad more defensive, so that we do not propagate invalid configuration (but issue stern warnings). JIRA: NETCONF-1237 Change-Id: Ib76c7ff6e3203e26c035e4ca475072a9d3964f77 Signed-off-by: Robert Varga --- diff --git a/keystore/keystore-legacy/src/main/java/org/opendaylight/netconf/keystore/legacy/AbstractNetconfKeystore.java b/keystore/keystore-legacy/src/main/java/org/opendaylight/netconf/keystore/legacy/AbstractNetconfKeystore.java index 0cae9c5998..3f98baea06 100644 --- a/keystore/keystore-legacy/src/main/java/org/opendaylight/netconf/keystore/legacy/AbstractNetconfKeystore.java +++ b/keystore/keystore-legacy/src/main/java/org/opendaylight/netconf/keystore/legacy/AbstractNetconfKeystore.java @@ -9,7 +9,14 @@ package org.opendaylight.netconf.keystore.legacy; import static java.util.Objects.requireNonNull; +import com.google.common.collect.Maps; +import java.nio.charset.StandardCharsets; +import java.security.GeneralSecurityException; +import java.security.cert.X509Certificate; +import java.util.ArrayList; +import java.util.Base64; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; @@ -23,17 +30,33 @@ import org.opendaylight.yang.gen.v1.urn.opendaylight.netconf.keystore.rev171017. import org.opendaylight.yang.gen.v1.urn.opendaylight.netconf.keystore.rev171017._private.keys.PrivateKey; import org.opendaylight.yang.gen.v1.urn.opendaylight.netconf.keystore.rev171017.trusted.certificates.TrustedCertificate; import org.opendaylight.yangtools.concepts.Immutable; +import org.opendaylight.yangtools.concepts.Mutable; import org.opendaylight.yangtools.concepts.Registration; import org.opendaylight.yangtools.yang.binding.InstanceIdentifier; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * Abstract substrate for implementing security services based on the contents of {@link Keystore}. */ public abstract class AbstractNetconfKeystore { + @NonNullByDefault + protected record CertifiedPrivateKey( + java.security.PrivateKey key, + List certificateChain) implements Immutable { + public CertifiedPrivateKey { + requireNonNull(key); + certificateChain = List.copyOf(certificateChain); + if (certificateChain.isEmpty()) { + throw new IllegalArgumentException("Certificate chain must not be empty"); + } + } + } + @NonNullByDefault protected record State( - Map privateKeys, - Map trustedCertificates) implements Immutable { + Map privateKeys, + Map trustedCertificates) implements Immutable { public static final State EMPTY = new State(Map.of(), Map.of()); public State { @@ -57,14 +80,17 @@ public abstract class AbstractNetconfKeystore { @NonNullByDefault record ConfigStateBuilder( HashMap privateKeys, - HashMap trustedCertificates) { + HashMap trustedCertificates) implements Mutable { ConfigStateBuilder { requireNonNull(privateKeys); requireNonNull(trustedCertificates); } } + private static final Logger LOG = LoggerFactory.getLogger(AbstractNetconfKeystore.class); + private final AtomicReference<@NonNull ConfigState> state = new AtomicReference<>(ConfigState.EMPTY); + private final SecurityHelper securityHelper = new SecurityHelper(); private @Nullable Registration configListener; @@ -100,10 +126,108 @@ public abstract class AbstractNetconfKeystore { return; } - // FIXME: compile to crypto + Throwable failure = null; + + final var keys = Maps.newHashMapWithExpectedSize(newState.privateKeys.size()); + for (var key : newState.privateKeys.values()) { + final var keyName = key.requireName(); + + final byte[] keyBytes; + try { + keyBytes = base64Decode(key.requireData()); + } catch (IllegalArgumentException e) { + LOG.debug("Failed to decode private key {}", keyName, e); + failure = updateFailure(failure, e); + continue; + } + + final java.security.PrivateKey privateKey; + try { + privateKey = securityHelper.generatePrivateKey(keyBytes); + } catch (GeneralSecurityException e) { + LOG.debug("Failed to generate key for {}", keyName, e); + failure = updateFailure(failure, e); + continue; + } + + final var certChain = key.requireCertificateChain(); + if (certChain.isEmpty()) { + LOG.debug("Key {} has an empty certificate chain", keyName); + failure = updateFailure(failure, + new IllegalArgumentException("Empty certificate chain for private key " + keyName)); + continue; + } + + final var certs = new ArrayList(certChain.size()); + for (int i = 0, size = certChain.size(); i < size; i++) { + final byte[] bytes; + try { + bytes = base64Decode(certChain.get(i)); + } catch (IllegalArgumentException e) { + LOG.debug("Failed to decode certificate chain item {} for private key {}", i, keyName, e); + failure = updateFailure(failure, e); + continue; + } + + final X509Certificate x509cert; + try { + x509cert = securityHelper.generateCertificate(bytes); + } catch (GeneralSecurityException e) { + LOG.debug("Failed to generate certificate chain item {} for private key {}", i, keyName, e); + failure = updateFailure(failure, e); + continue; + } + + certs.add(x509cert); + } + + keys.put(keyName, new CertifiedPrivateKey(privateKey, certs)); + } + + final var certs = Maps.newHashMapWithExpectedSize(newState.trustedCertificates.size()); + for (var cert : newState.trustedCertificates.values()) { + final var certName = cert.requireName(); + + final byte[] bytes; + try { + bytes = base64Decode(cert.requireCertificate()); + } catch (IllegalArgumentException e) { + LOG.debug("Failed to decode trusted certificate {}", certName, e); + failure = updateFailure(failure, e); + continue; + } + + final X509Certificate x509cert; + try { + x509cert = securityHelper.generateCertificate(bytes); + } catch (GeneralSecurityException e) { + LOG.debug("Failed to generate certificate for {}", certName, e); + failure = updateFailure(failure, e); + continue; + } + + certs.put(certName, x509cert); + } - onStateUpdated(new State(newState.privateKeys, newState.trustedCertificates)); + if (failure != null) { + LOG.warn("New configuration is invalid, not applying it", failure); + return; + } + + onStateUpdated(new State(keys, certs)); // FIXME: tickle operational updater (which does not exist yet) } + + private static byte[] base64Decode(final String base64) { + return Base64.getMimeDecoder().decode(base64.getBytes(StandardCharsets.US_ASCII)); + } + + private static @NonNull Throwable updateFailure(final @Nullable Throwable failure, final @NonNull Exception ex) { + if (failure != null) { + failure.addSuppressed(ex); + return failure; + } + return ex; + } } diff --git a/keystore/keystore-legacy/src/main/java/org/opendaylight/netconf/keystore/legacy/SecurityHelper.java b/keystore/keystore-legacy/src/main/java/org/opendaylight/netconf/keystore/legacy/SecurityHelper.java index 621240bbc2..18b3b547da 100644 --- a/keystore/keystore-legacy/src/main/java/org/opendaylight/netconf/keystore/legacy/SecurityHelper.java +++ b/keystore/keystore-legacy/src/main/java/org/opendaylight/netconf/keystore/legacy/SecurityHelper.java @@ -8,7 +8,6 @@ package org.opendaylight.netconf.keystore.legacy; import java.io.ByteArrayInputStream; -import java.nio.charset.StandardCharsets; import java.security.GeneralSecurityException; import java.security.KeyFactory; import java.security.PrivateKey; @@ -16,16 +15,15 @@ import java.security.cert.CertificateFactory; import java.security.cert.X509Certificate; import java.security.spec.InvalidKeySpecException; import java.security.spec.PKCS8EncodedKeySpec; -import java.util.Base64; import org.eclipse.jdt.annotation.NonNull; -public final class SecurityHelper { +final class SecurityHelper { private CertificateFactory certFactory; private KeyFactory dsaFactory; private KeyFactory rsaFactory; - public @NonNull PrivateKey getJavaPrivateKey(final String base64PrivateKey) throws GeneralSecurityException { - final var keySpec = new PKCS8EncodedKeySpec(base64Decode(base64PrivateKey)); + @NonNull PrivateKey generatePrivateKey(final byte[] privateKey) throws GeneralSecurityException { + final var keySpec = new PKCS8EncodedKeySpec(privateKey); if (rsaFactory == null) { rsaFactory = KeyFactory.getInstance("RSA"); @@ -42,18 +40,12 @@ public final class SecurityHelper { return dsaFactory.generatePrivate(keySpec); } - public @NonNull X509Certificate getCertificate(final String base64Certificate) throws GeneralSecurityException { + @NonNull X509Certificate generateCertificate(final byte[] certificate) throws GeneralSecurityException { // TODO: https://stackoverflow.com/questions/43809909/is-certificatefactory-getinstancex-509-thread-safe // indicates this is thread-safe in most cases, but can we get a better assurance? if (certFactory == null) { certFactory = CertificateFactory.getInstance("X.509"); } - return (X509Certificate) certFactory.generateCertificate( - new ByteArrayInputStream(base64Decode(base64Certificate))); + return (X509Certificate) certFactory.generateCertificate(new ByteArrayInputStream(certificate)); } - - private static byte[] base64Decode(final String base64) { - return Base64.getMimeDecoder().decode(base64.getBytes(StandardCharsets.US_ASCII)); - } - } \ No newline at end of file diff --git a/plugins/netconf-client-mdsal/src/main/java/org/opendaylight/netconf/client/mdsal/impl/DefaultSslHandlerFactoryProvider.java b/plugins/netconf-client-mdsal/src/main/java/org/opendaylight/netconf/client/mdsal/impl/DefaultSslHandlerFactoryProvider.java index 64752f6649..0ff1359374 100644 --- a/plugins/netconf-client-mdsal/src/main/java/org/opendaylight/netconf/client/mdsal/impl/DefaultSslHandlerFactoryProvider.java +++ b/plugins/netconf-client-mdsal/src/main/java/org/opendaylight/netconf/client/mdsal/impl/DefaultSslHandlerFactoryProvider.java @@ -13,8 +13,7 @@ import java.io.IOException; import java.security.GeneralSecurityException; import java.security.KeyStore; import java.security.KeyStoreException; -import java.security.cert.Certificate; -import java.security.cert.CertificateException; +import java.security.cert.X509Certificate; import java.util.Set; import javax.annotation.PreDestroy; import javax.inject.Inject; @@ -24,7 +23,6 @@ import org.opendaylight.mdsal.binding.api.DataBroker; import org.opendaylight.netconf.client.SslHandlerFactory; import org.opendaylight.netconf.client.mdsal.api.SslHandlerFactoryProvider; import org.opendaylight.netconf.keystore.legacy.AbstractNetconfKeystore; -import org.opendaylight.netconf.keystore.legacy.SecurityHelper; import org.opendaylight.yang.gen.v1.urn.opendaylight.netconf.device.rev240120.connection.parameters.protocol.Specification; import org.opendaylight.yang.gen.v1.urn.opendaylight.netconf.device.rev240120.connection.parameters.protocol.specification.TlsCase; import org.osgi.service.component.annotations.Activate; @@ -36,6 +34,7 @@ import org.osgi.service.component.annotations.Reference; @Component(service = SslHandlerFactoryProvider.class) public final class DefaultSslHandlerFactoryProvider extends AbstractNetconfKeystore implements SslHandlerFactoryProvider, AutoCloseable { + private static final X509Certificate[] EMPTY_CERTS = { }; private static final char[] EMPTY_CHARS = { }; private final @NonNull SslHandlerFactory nospecFactory = new SslHandlerFactoryImpl(this, Set.of()); @@ -107,33 +106,18 @@ public final class DefaultSslHandlerFactoryProvider extends AbstractNetconfKeyst final var keyStore = KeyStore.getInstance("JKS"); keyStore.load(null, null); - final var helper = new SecurityHelper(); - // Private keys first for (var entry : current.privateKeys().entrySet()) { final var alias = entry.getKey(); - if (!allowedKeys.isEmpty() && !allowedKeys.contains(alias)) { - continue; - } - - final var privateKey = entry.getValue(); - final var key = helper.getJavaPrivateKey(privateKey.getData()); - // TODO: requireCertificateChain() here and filter in update path - final var certChain = privateKey.getCertificateChain(); - if (certChain == null || certChain.isEmpty()) { - throw new CertificateException("No certificate chain associated with private key " + alias + " found"); - } - - final var chain = new Certificate[certChain.size()]; - int idx = 0; - for (var cert : certChain) { - chain[idx++] = helper.getCertificate(cert); + if (allowedKeys.isEmpty() || allowedKeys.contains(alias)) { + final var privateKey = entry.getValue(); + keyStore.setKeyEntry(alias, privateKey.key(), EMPTY_CHARS, + privateKey.certificateChain().toArray(EMPTY_CERTS)); } - keyStore.setKeyEntry(alias, key, EMPTY_CHARS, chain); } for (var entry : current.trustedCertificates().entrySet()) { - keyStore.setCertificateEntry(entry.getKey(), helper.getCertificate(entry.getValue().getCertificate())); + keyStore.setCertificateEntry(entry.getKey(), entry.getValue()); } return keyStore;