Skip to content

fix: Presto spark add X509 certificates to SessionContext #27183

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.

Sign up for GitHub

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

Open
kevintang2022 wants to merge 1 commit into
base: master
Choose a base branch
from
+51 −11
Open

fix: Presto spark add X509 certificates to SessionContext #27183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion presto-spark-base/src/main/java/com/facebook/presto/spark/PrestoSparkSessionContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
import com.google.common.collect.ImmutableSet;
import jakarta.annotation.Nullable;

import java.security.cert.X509Certificate;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
Expand Down Expand Up @@ -77,7 +79,8 @@ public static PrestoSparkSessionContext createFromSessionInfo(
extraCredentials.build(),
extraTokenAuthenticators.build(),
Optional.empty(),
Optional.empty()),
Optional.empty(),
prestoSparkSession.getCertificates()),
prestoSparkSession.getCatalog().orElse(null),
prestoSparkSession.getSchema().orElse(null),
prestoSparkSession.getSource().orElse(null),
Expand Down Expand Up @@ -128,6 +131,12 @@ public Identity getIdentity()
return identity;
}

@Override
public List<X509Certificate> getCertificates()
{
return identity.getCertificates();
Copy link
Contributor

@singcha singcha Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can it return null?

Copy link
Contributor Author

@kevintang2022 kevintang2022 Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can never be null. The getCertificates() method on Identity class always returns a list

Copy link
Contributor Author

@kevintang2022 kevintang2022 Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PrestoSparkSessionContext constructor also require non null Identity

}

@Nullable
@Override
public String getCatalog()
Expand Down
19 changes: 10 additions & 9 deletions presto-spark-base/src/test/java/com/facebook/presto/spark/PrestoSparkQueryRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,15 +339,15 @@ public PrestoSparkQueryRunner(
// Sql-Standard Access Control Checker
// needs us to specify our role
.setIdentity(
new Identity(
"hive",
Optional.empty(),
ImmutableMap.of(defaultCatalog,
new SelectedRole(Type.ROLE, Optional.of("admin"))),
ImmutableMap.of(),
ImmutableMap.of(),
Optional.empty(),
Optional.empty()))
new Identity(
"hive",
Optional.empty(),
ImmutableMap.of(defaultCatalog,
new SelectedRole(Type.ROLE, Optional.of("admin"))),
ImmutableMap.of(),
ImmutableMap.of(),
Optional.empty(),
Optional.empty()))
.build();

transactionManager = injector.getInstance(TransactionManager.class);
Expand Down Expand Up @@ -659,6 +659,7 @@ private static PrestoSparkSession createSessionInfo(Session session)
session.getIdentity().getUser(),
session.getIdentity().getPrincipal(),
session.getIdentity().getExtraCredentials(),
session.getIdentity().getCertificates(),
Copy link
Contributor

@sourcery-ai sourcery-ai bot Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Add a test that verifies X509 certificates are propagated end-to-end into the Spark session/context

The new getCertificates() plumbing is threaded through PrestoSparkSession and PrestoSparkSessionContext, but there’s no test confirming that a Session with non-empty certificates yields the same certificates on the Spark side (e.g., via PrestoSparkSessionContext.getIdentity().getCertificates() or whatever surface is used during Spark query execution). Please add an integration/e2e test that:

  • Builds a Session/Identity with one or more test X509Certificate instances.
  • Runs a simple Spark query via the existing Presto Spark test harness (e.g., PrestoSparkQueryRunner).
  • Asserts the certificates are visible in the Spark execution path via the appropriate context.

This validates that the new field is not only set at construction but is actually available where authentication will rely on it.

Suggested implementation:

                session.getIdentity().getExtraCredentials(),
                session.getIdentity().getCertificates(),
                session.getCatalog(),
                session.getSchema(),
                session.getSource(),

    @Test
    public void testX509CertificatesPropagatedToSparkSessionContext()
            throws Exception
    {
        // Arrange: build a Session with a non-empty certificates list
        X509Certificate certificate = mock(X509Certificate.class);
        List<X509Certificate> certificates = ImmutableList.of(certificate);

        Session sessionWithCertificates = createSessionBuilder()
                .setIdentity(
                        Identity.forUser("test_user")
                                .withPrincipal("test_principal")
                                .withCertificates(certificates)
                                .build())
                .build();

        // Act: run a simple Spark query using the existing Presto Spark test harness
        PrestoSparkQueryRunner queryRunner = createPrestoSparkQueryRunner();
        MaterializedResult result = queryRunner.execute(sessionWithCertificates, "SELECT 1");

        // Sanity check query executed successfully
        assertEquals(result.getOnlyValue(), 1L);

        // Assert: the certificates are visible from the Spark execution context
        PrestoSparkSessionContext sparkSessionContext = queryRunner.getLastSparkSessionContext();
        assertNotNull(sparkSessionContext, "Spark session context should be initialized");
        assertEquals(
                sparkSessionContext.getIdentity().getCertificates(),
                certificates,
                "X509 certificates should be propagated end-to-end into the Spark session context");
    }

To make the above compile and behave as intended, you will also need to:

  1. Add imports at the top of this file (or adjust existing ones) for:

    • org.testng.annotations.Test (or JUnit equivalent, depending on the rest of the file).
    • java.security.cert.X509Certificate
    • java.util.List
    • com.google.common.collect.ImmutableList
    • static org.testng.Assert.assertEquals
    • static org.testng.Assert.assertNotNull
    • static org.mockito.Mockito.mock (if Mockito is already used in the test suite)
    • com.facebook.presto.Session
    • com.facebook.presto.spi.security.Identity
    • com.facebook.presto.testing.MaterializedResult
    • com.facebook.presto.spark.classloader_interface.PrestoSparkSessionContext (or the correct package for PrestoSparkSessionContext in your tree)

  2. Provide or reuse helpers referenced in the test:

    • createSessionBuilder() – if not already present, this should return a Session.SessionBuilder configured consistently with other tests in this class (e.g., using an existing SessionPropertyManager and default catalog/schema).
    • createPrestoSparkQueryRunner() – if this file is itself the query runner, replace this call with the appropriate factory/constructor used elsewhere (for example new PrestoSparkQueryRunner(...) or a static factory).
    • getLastSparkSessionContext() – you will need to expose the last PrestoSparkSessionContext used during query execution from PrestoSparkQueryRunner. A typical pattern is:
      • Add a volatile PrestoSparkSessionContext lastSparkSessionContext; field.
      • In the code path where you construct the PrestoSparkSessionContext (the block containing the session.getIdentity().getUser(), getPrincipal(), getExtraCredentials(), getCertificates(), etc.), assign it to lastSparkSessionContext.
      • Implement public PrestoSparkSessionContext getLastSparkSessionContext() that returns this field.
    • If the class is not currently a TestNG/JUnit test class, either:
      • Move the new test method into an appropriate Test* class that has access to the query runner, or
      • Annotate this class as a test class following your existing test framework conventions.

  3. Align Identity API calls:

    • The example uses Identity.forUser(...).withPrincipal(...).withCertificates(...).build(). Adjust to the actual API of Identity in your codebase (e.g., use a builder or constructor you already rely on; the key requirement is that the resulting Identity returns the certificates list from getCertificates()).

  4. Align execute signature and result assertion:

    • If PrestoSparkQueryRunner.execute(...) has a different signature or return type than MaterializedResult, adjust the test to match (the objective is only to run a simple query and verify the session context, so any trivial query is fine).

session.getCatalog(),
session.getSchema(),
session.getSource(),
Expand Down
12 changes: 12 additions & 0 deletions ...ace/src/main/java/com/facebook/presto/spark/classloader_interface/PrestoSparkSession.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,16 @@
package com.facebook.presto.spark.classloader_interface;

import java.security.Principal;
import java.security.cert.X509Certificate;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;

import static java.util.Collections.unmodifiableList;
import static java.util.Collections.unmodifiableMap;
import static java.util.Collections.unmodifiableSet;
import static java.util.Objects.requireNonNull;
Expand All @@ -33,6 +37,7 @@ public class PrestoSparkSession
private final String user;
private final Optional<Principal> principal;
private final Map<String, String> extraCredentials;
private final List<X509Certificate> certificates;
private final Optional<String> catalog;
private final Optional<String> schema;
private final Optional<String> source;
Expand All @@ -49,6 +54,7 @@ public PrestoSparkSession(
String user,
Optional<Principal> principal,
Map<String, String> extraCredentials,
List<X509Certificate> certificates,
Optional<String> catalog,
Optional<String> schema,
Optional<String> source,
Expand All @@ -65,6 +71,7 @@ public PrestoSparkSession(
this.user = requireNonNull(user, "user is null");
this.principal = requireNonNull(principal, "principal is null");
this.extraCredentials = unmodifiableMap(new HashMap<>(requireNonNull(extraCredentials, "extraCredentials is null")));
this.certificates = unmodifiableList(new ArrayList<>(requireNonNull(certificates, "certificates is null")));
this.catalog = requireNonNull(catalog, "catalog is null");
this.schema = requireNonNull(schema, "schema is null");
this.source = requireNonNull(source, "source is null");
Expand Down Expand Up @@ -94,6 +101,11 @@ public Map<String, String> getExtraCredentials()
return extraCredentials;
}

public List<X509Certificate> getCertificates()
{
return certificates;
}

public Optional<String> getCatalog()
{
return catalog;
Expand Down
4 changes: 3 additions & 1 deletion ...launcher/src/main/java/com/facebook/presto/spark/launcher/PrestoSparkLauncherCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package com.facebook.presto.spark.launcher;

import com.facebook.presto.spark.classloader_interface.PrestoSparkConfInitializer;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import io.airlift.airline.Command;
Expand Down Expand Up @@ -67,14 +68,15 @@ public void run()
Optional.empty(),
clientOptions.sessionPropertyConfig == null ? Optional.empty() : Optional.of(
loadProperties(checkFile(new File(clientOptions.sessionPropertyConfig)))),
Optional.empty(),
Optional.empty(),
Optional.empty());

try (PrestoSparkRunner runner = new PrestoSparkRunner(distribution)) {
runner.run(
"test",
Optional.empty(),
ImmutableMap.of(),
ImmutableList.of(),
clientOptions.catalog,
clientOptions.schema,
Optional.empty(),
Expand Down
4 changes: 4 additions & 0 deletions ...to-spark-launcher/src/main/java/com/facebook/presto/spark/launcher/PrestoSparkRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import java.net.MalformedURLException;
import java.net.URL;
import java.security.Principal;
import java.security.cert.X509Certificate;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -84,6 +85,7 @@ public void run(
String user,
Optional<Principal> principal,
Map<String, String> extraCredentials,
List<X509Certificate> certificates,
String catalog,
String schema,
Optional<String> source,
Expand All @@ -106,6 +108,7 @@ public void run(
user,
principal,
extraCredentials,
certificates,
catalog,
schema,
source,
Expand Down Expand Up @@ -154,6 +157,7 @@ private void execute(IPrestoSparkQueryExecutionFactory queryExecutionFactory, Pr
prestoSparkRunnerContext.getUser(),
prestoSparkRunnerContext.getPrincipal(),
prestoSparkRunnerContext.getExtraCredentials(),
prestoSparkRunnerContext.getCertificates(),
Optional.ofNullable(prestoSparkRunnerContext.getCatalog()),
Optional.ofNullable(prestoSparkRunnerContext.getSchema()),
prestoSparkRunnerContext.getSource(),
Expand Down
12 changes: 12 additions & 0 deletions ...k-launcher/src/main/java/com/facebook/presto/spark/launcher/PrestoSparkRunnerContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import com.facebook.presto.spark.classloader_interface.ExecutionStrategy;

import java.security.Principal;
import java.security.cert.X509Certificate;
import java.util.List;
import java.util.Map;
import java.util.Optional;
Expand All @@ -28,6 +29,7 @@ public class PrestoSparkRunnerContext
private final String user;
private final Optional<Principal> principal;
private final Map<String, String> extraCredentials;
private final List<X509Certificate> certificates;
private final String catalog;
private final String schema;
private final Optional<String> source;
Expand All @@ -50,6 +52,7 @@ public PrestoSparkRunnerContext(
String user,
Optional<Principal> principal,
Map<String, String> extraCredentials,
List<X509Certificate> certificates,
String catalog,
String schema,
Optional<String> source,
Expand All @@ -71,6 +74,7 @@ public PrestoSparkRunnerContext(
this.user = user;
this.principal = principal;
this.extraCredentials = extraCredentials;
this.certificates = certificates;
this.catalog = catalog;
this.schema = schema;
this.source = source;
Expand Down Expand Up @@ -105,6 +109,11 @@ public Map<String, String> getExtraCredentials()
return extraCredentials;
}

public List<X509Certificate> getCertificates()
{
return certificates;
}

public String getCatalog()
{
return catalog;
Expand Down Expand Up @@ -195,6 +204,7 @@ public static class Builder
private String user;
private Optional<Principal> principal;
private Map<String, String> extraCredentials;
private List<X509Certificate> certificates;
private String catalog;
private String schema;
private Optional<String> source;
Expand All @@ -218,6 +228,7 @@ public Builder(PrestoSparkRunnerContext prestoSparkRunnerContext)
this.user = prestoSparkRunnerContext.getUser();
this.principal = prestoSparkRunnerContext.getPrincipal();
this.extraCredentials = prestoSparkRunnerContext.getExtraCredentials();
this.certificates = prestoSparkRunnerContext.getCertificates();
this.catalog = prestoSparkRunnerContext.getCatalog();
this.schema = prestoSparkRunnerContext.getSchema();
this.source = prestoSparkRunnerContext.getSource();
Expand Down Expand Up @@ -249,6 +260,7 @@ public PrestoSparkRunnerContext build()
user,
principal,
extraCredentials,
certificates,
catalog,
schema,
source,
Expand Down
Loading