Remove Bouncy Castle dependency usage from PemUtils #1318

exceptionfactory · 2025-04-05T17:01:58Z

This pull request removes use of the Bouncy Castle Provider library for the PemUtils class for parsing public and private key files.

The Bouncy Castle library includes a large number of cryptographic capabilities, some of which are duplicative of current capabilities in recent versions of Java. Parsing PEM-encoded files is the only direct use of Bouncy Castle in Polaris based on imported classes, so replacing this usage with an alternative solution removes the need for a significant direct dependency.

The PemUtils class already uses standard Java Security components for parsing decoded public keys and private key material, so the changes are scoped to replacing the initial PEM content parsing and Base64 decoding.

The changes include a new PemUtilsTest class that exercises parsing RSA public key and private key files.

The PemUtils class usage is limited to JWT asymmetric token support, narrowing the scope of changes to this specific token signing implementation.

adutra · 2025-04-07T12:16:13Z

service/common/src/main/java/org/apache/polaris/service/auth/PemUtils.java

@@ -46,9 +45,38 @@ private static byte[] parsePEMFile(Path pemPath) throws IOException {
      throw new FileNotFoundException(
          String.format("The file '%s' doesn't exist.", pemPath.toAbsolutePath()));
    }
-    try (PemReader reader = new PemReader(Files.newBufferedReader(pemPath, UTF_8))) {
-      PemObject pemObject = reader.readPemObject();


One general remark: the original readPemObject() method, in principle, only reads the first object in the PEM file:

https://downloads.bouncycastle.org/java/docs/bcprov-jdk18on-javadoc/org/bouncycastle/util/io/pem/PemReader.html#readPemObject()

With your changes, what happens when the PEM file contains more than one object? And what if it doesn't contain any? It might be good to add tests for these cases as well.

Thanks for the feedback and noting the behavior of readPemObject(). I adjusted the new implementation to stop reading when finding a footer line, and added unit tests for that scenario, as well as an empty file, which throws an IOException.

snazy

The LICENSE/NOTICE changes need to be corrected.

snazy · 2025-04-07T12:34:48Z

quarkus/server/distribution/LICENSE

@@ -2050,12 +2050,6 @@ License (from POM): Apache License 2.0 - https://www.apache.org/licenses/LICENSE

 --------------------------------------------------------------------------------

-Group: org.bouncycastle Name: bcprov-jdk18on Version: 1.80


bouncycastle/bc-prov is still a dependency of polaris-quarkus-server (see dependencies of the runtimeClasspath Gradle configuration), so the removals in LICENSE/NOTICE are wrong.

Thanks for noting the runtime transitive dependency on bcprov-jdk18on, I reverted the license and notice changes.

exceptionfactory · 2025-04-07T12:55:50Z

Thanks for the feedback @adutra and @snazy, I pushed updates to address your comments thus far.

dimas-b

LGTM 👍 Thanks, @exceptionfactory !

exceptionfactory · 2025-04-14T12:52:24Z

Thanks for the feedback @dimas-b!

Do you have any additional comments @snazy or @adutra following the updates?

adutra · 2025-04-18T09:08:25Z

I'm good here; @snazy do you have further remarks?

snazy

License file concerns are addressed, so I'm okay.

- Added PEM format parsing in PemUtils - Added unit test for PemUtils - Removed Bouncy Castle Provider dependency from service common module - Removed Bouncy Castle Provider dependency from quarkus service module - Removed Bouncy Castle references from LICENSE and NOTICE files

exceptionfactory · 2025-05-20T13:53:43Z

Thanks for the updated review @snazy. I force-pushed an update to resolve recent merge conflicts. Please let me know if there is anything else that needs to be addressed.

snazy · 2025-05-20T14:55:38Z

Thanks @exceptionfactory

* main: Update docker.io/prom/prometheus Docker tag to v3.4.0 (apache#1602) * Site: Update production configuration page (apache#1606) * main: Update dependency com.google.cloud:google-cloud-storage-bom to v2.52.3 (apache#1623) * main: Update dependency boto3 to v1.38.19 (apache#1622) * Remove Bouncy Castle dependency usage from PemUtils (apache#1318) - Added PEM format parsing in PemUtils - Added unit test for PemUtils for empty file and multiple PEM objects - Removed Bouncy Castle Provider dependency from service common module - Removed Bouncy Castle Provider dependency from quarkus service module * Site: Add a page for policy management (apache#1600) * [Policy Store | Management Spec] Add policy privileges to spec and update admin service impl (apache#1529) This PR adds new policy related privileges to polaris-management-api.yml and update PolarisAdminService to allow granting new privileges * Spec: Add SigV4 Auth Support for Catalog Federation (apache#1506) * Spec changes for SigV4 Auth Support for Catalog Federation * Extract service identity info as a nested object * nit: fix admin tool log level and comments (apache#1626) The previous WARNING log levels seems to work, but WARN aligns better with standard Quarkus log levels. Fixes apache#1612 * Doc: switch to use iceberg-aws-bundle jar (apache#1609) * main: Update dependency org.mockito:mockito-core to v5.18.0 (apache#1630) * main: Update dependency boto3 to v1.38.20 (apache#1631) * Require explicit user-consent to enable HadoopFileIO (apache#1532) Using `HadoopFileIO` in Polaris can enable "hidden features" that users are likely not aware of. This change requires users to manually update the configuration to be able to use `HadoopFileIO` in way that highlights the consequences of enabling it. This PR updates Polaris in multiple ways: * The default of `SUPPORTED_CATALOG_STORAGE_TYPES` is changed to not include the `FILE` storage type. * Respect the `ALLOW_SPECIFYING_FILE_IO_IMPL` configuration on namespaces, tables and views to prevent setting an `io-impl` value for anything but one of the configured, supported storage-types. * Unify validation code in a new class `IcebergPropertiesValidation`. * Using `FILE` or `HadoopFileIO` now _also_ requires the explicit configuration `ALLOW_INSECURE_STORAGE_TYPES_ACCEPTING_SECURITY_RISKS=true`. * Added production readiness checks that trigger when `ALLOW_INSECURE_STORAGE_TYPES_ACCEPTING_SECURITY_RISKS` is `true` or `SUPPORTED_CATALOG_STORAGE_TYPES` contains `FILE` (defaults and per-realm). * The two new readiness checks are considered _severe_. Severe readiness-errors prevent the server from starting up - unless the user explicitly configured `polaris.readiness.ignore-security-issues=true`. Log messages and configuration options explicitly use "clear" phrases highlighting the consequences. With these changes it is intentionally extremely difficult to start Polaris with HadoopFileIO. People who work around all these safety nets must have realized that what they are doing. A lot of the test code relies on `FILE`/`HadoopFileIO`, those tests got all the configurations to let those tests continue to work as they are, bypassing the added security safeguards. --------- Co-authored-by: Dmitri Bourlatchkov <dmitri.bourlatchkov@dremio.com> --------- Co-authored-by: Mend Renovate <bot@renovateapp.com> Co-authored-by: Yufei Gu <yufei@apache.org> Co-authored-by: David Handermann <exceptionfactory@apache.org> Co-authored-by: Honah (Jonas) J. <honahx@apache.org> Co-authored-by: Rulin Xing <xjdkcsq3@gmail.com> Co-authored-by: Dmitri Bourlatchkov <dmitri.bourlatchkov@dremio.com> Co-authored-by: MonkeyCanCode <yongzheng0809@gmail.com>

- Added PEM format parsing in PemUtils - Added unit test for PemUtils for empty file and multiple PEM objects - Removed Bouncy Castle Provider dependency from service common module - Removed Bouncy Castle Provider dependency from quarkus service module

exceptionfactory requested review from adutra, ashvina, dennishuo, dimas-b, eric-maynard, jackye1995, jbonofre, vvcephei, collado-mike and snazy as code owners April 5, 2025 17:01

github-project-automation bot added this to Basic Kanban Board Apr 5, 2025

exceptionfactory requested review from RussellSpitzer, takidau, MonkeyCanCode, flyrain and ebyhr as code owners April 5, 2025 17:02

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Apr 5, 2025

adutra previously approved these changes Apr 7, 2025

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Apr 7, 2025

adutra reviewed Apr 7, 2025

View reviewed changes

snazy requested changes Apr 7, 2025

View reviewed changes

github-project-automation bot moved this from Ready to merge to PRs In Progress in Basic Kanban Board Apr 7, 2025

exceptionfactory dismissed adutra’s stale review via c1aa0bb April 7, 2025 12:53

dimas-b previously approved these changes Apr 7, 2025

View reviewed changes

snazy reviewed May 19, 2025

View reviewed changes

exceptionfactory added 2 commits May 20, 2025 08:48

Reverted LICENSE and NOTICE changes to retain Bouncy Castle

abf595c

Added tests for empty file and multiple PEM objects

16aa2e0

exceptionfactory dismissed dimas-b’s stale review via 76049b3 May 20, 2025 13:49

exceptionfactory force-pushed the remove-bouncycastle-pem-1 branch from cf06a5e to 76049b3 Compare May 20, 2025 13:49

exceptionfactory requested review from HonahX, singhpk234 and pingtimeout as code owners May 20, 2025 13:49

Applied spotless formatting

29525ec

exceptionfactory force-pushed the remove-bouncycastle-pem-1 branch from 76049b3 to 29525ec Compare May 20, 2025 13:51

snazy approved these changes May 20, 2025

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board May 20, 2025

snazy merged commit 5332e4f into apache:main May 20, 2025
6 checks passed

github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board May 20, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove Bouncy Castle dependency usage from PemUtils #1318

Remove Bouncy Castle dependency usage from PemUtils #1318

Uh oh!

exceptionfactory commented Apr 5, 2025

Uh oh!

adutra Apr 7, 2025

Uh oh!

exceptionfactory Apr 7, 2025

Uh oh!

snazy left a comment

Uh oh!

snazy Apr 7, 2025

Uh oh!

exceptionfactory Apr 7, 2025

Uh oh!

exceptionfactory commented Apr 7, 2025

Uh oh!

dimas-b left a comment

Uh oh!

exceptionfactory commented Apr 14, 2025

Uh oh!

adutra commented Apr 18, 2025

Uh oh!

snazy left a comment

Uh oh!

exceptionfactory commented May 20, 2025

Uh oh!

Uh oh!

snazy commented May 20, 2025

Uh oh!

Uh oh!

		@@ -2050,12 +2050,6 @@ License (from POM): Apache License 2.0 - https://www.apache.org/licenses/LICENSE

		--------------------------------------------------------------------------------

		Group: org.bouncycastle Name: bcprov-jdk18on Version: 1.80

Remove Bouncy Castle dependency usage from PemUtils #1318

Remove Bouncy Castle dependency usage from PemUtils #1318

Uh oh!

Conversation

exceptionfactory commented Apr 5, 2025

Uh oh!

adutra Apr 7, 2025

Choose a reason for hiding this comment

Uh oh!

exceptionfactory Apr 7, 2025

Choose a reason for hiding this comment

Uh oh!

snazy left a comment

Choose a reason for hiding this comment

Uh oh!

snazy Apr 7, 2025

Choose a reason for hiding this comment

Uh oh!

exceptionfactory Apr 7, 2025

Choose a reason for hiding this comment

Uh oh!

exceptionfactory commented Apr 7, 2025

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

exceptionfactory commented Apr 14, 2025

Uh oh!

adutra commented Apr 18, 2025

Uh oh!

snazy left a comment

Choose a reason for hiding this comment

Uh oh!

exceptionfactory commented May 20, 2025

Uh oh!

Uh oh!

snazy commented May 20, 2025

Uh oh!

Uh oh!