Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add trustStore and needClientAuth config to yaml (#834) #1118

Merged
merged 2 commits into from
Jan 31, 2025

Conversation

gtully
Copy link
Contributor

@gtully gtully commented Jan 14, 2025

@dhoard I am making use of the custom authenticator and want to grant permissions based on the clients certificate so I need to be able to configure needClientAuth on the ssl parameters. Also, in kube land, I need to be able to configure the key and trust store types to support PEM key stores. Having these exposed in the yaml config leaves the system properties free for others.

From what I understand, this additional truststore config will also help with #834

@gtully
Copy link
Contributor Author

gtully commented Jan 14, 2025

peeking at that test failure.. sorted!

@gtully
Copy link
Contributor Author

gtully commented Jan 14, 2025

if it helps, for some context, the yaml for this use case is produced at: https://github.com/gtully/activemq-artemis-operator/blob/_jmx_exporter_agent/controllers/activemqartemis_reconciler.go#L2096

@gtully
Copy link
Contributor Author

gtully commented Jan 20, 2025

@dhoard thanks for the review, much appreciated.

@dhoard
Copy link
Collaborator

dhoard commented Jan 29, 2025

@gtully Initial comments...

  1. Since this enables mutual TLS, I feel we should change the configuration from needClientAuth to mutualTLS
httpServer:
  ssl:
    mutualTLS: true
    keyStore:
      type: PKCS12
      filename: localhost.pkcs12
      password: changeit
    trustStore:
      type: PKCS12
      filename: localhost.pkcs12
      password: changeit
  1. We can simplify the logic to get the configuration value by creating a new ConvertToBoolean class.
package io.prometheus.jmx.common.configuration;

import io.prometheus.jmx.common.util.Precondition;
import java.util.function.Function;
import java.util.function.Supplier;

/** Class to implement ConvertToBoolean */
public class ConvertToBoolean implements Function<Object, Boolean> {

    private final Supplier<? extends RuntimeException> supplier;

    /**
     * Constructor
     *
     * @param supplier supplier
     */
    public ConvertToBoolean(Supplier<? extends RuntimeException> supplier) {
        Precondition.notNull(supplier);
        this.supplier = supplier;
    }

    /**
     * Method to apply a function
     *
     * @param value value
     * @return the return value
     */
    @Override
    public Boolean apply(Object value) {
        if (value == null) {
            throw new IllegalArgumentException();
        }

        try {
            if (value instanceof Boolean) {
                return (Boolean) value;
            } else {
                return Boolean.valueOf(value.toString());
            }
        } catch (Throwable t) {
            throw supplier.get();
        }
    }
}

Usage

                boolean mutualTLS =
                        rootYamlMapAccessor
                                .get("/httpServer/ssl/mutualTLS")
                                .map(
                                        new ConvertToString(
                                                ConfigurationException.supplier(
                                                        "Invalid configuration for"
                                                                + " /httpServer/ssl/needClientAuth"
                                                                + " must be a boolean")))
                                .map(
                                        new ValidateStringIsNotBlank(
                                                ConfigurationException.supplier(
                                                        "Invalid configuration for"
                                                                + " /httpServer/ssl/mutualTLS"
                                                                + " must not be blank")))
                                .map(
                                        new ConvertToBoolean(
                                                ConfigurationException.supplier(
                                                        "Invalid configuration for"
                                                                + " /httpServer/ssl/mutualTLS"
                                                                + " must be a boolean")))
                                .orElse(false);

if (mutualTLS) {

// ... code to set up the truststore variables, configure the Http server, etc.

  1. The integration test class SSLWithTrustStoreAndClientAuth needs to have both positive and negative tests for all the scenarios for full test coverage
@Verifyica.Test
public void testHealthy(ExporterTestEnvironment exporterTestEnvironment) throws IOException
@Verifyica.Test
public void testDefaultTextMetrics(ExporterTestEnvironment exporterTestEnvironment) throws IOException
@Verifyica.Test
public void testOpenMetricsTextMetrics(ExporterTestEnvironment exporterTestEnvironment) throws IOException
@Verifyica.Test
public void testPrometheusTextMetrics(ExporterTestEnvironment exporterTestEnvironment) throws IOException
@Verifyica.Test
public void testPrometheusProtobufMetrics(ExporterTestEnvironment exporterTestEnvironment) throws IOException

@gtully
Copy link
Contributor Author

gtully commented Jan 30, 2025

@dhoard sure, mutualTLS works too and is more high level, thanks.
I incorporated your ConvertToBoolean idea and repeated the with and without client cert pattern on each of the test scenarios. I am not sure of the value because the tls layer is doing the real work. But it is more consistent I guess. Thanks for your time on this.

Updated copyright year

Signed-off-by: Doug Hoard <[email protected]>
@dhoard
Copy link
Collaborator

dhoard commented Jan 31, 2025

@gtully looks good! Thanks again for the PR!!!

@dhoard dhoard merged commit 27be7ef into prometheus:main Jan 31, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants