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

Quarkus 3.5.0.CR1 + Don't use VertxRecorder to get Vertx instance #182

Merged
merged 1 commit into from
Oct 11, 2023

Conversation

vsevel
Copy link
Contributor

@vsevel vsevel commented Oct 11, 2023

supersedes #167 @geoand
relates to quarkusio/quarkus-platform#1002 (comment) @gsmet
strangely, I had to rework the assertions on the pki test. what do you think @kdubb ?

@vsevel vsevel requested a review from kdubb October 11, 2023 17:01
@kdubb
Copy link
Contributor

kdubb commented Oct 11, 2023

The changes to assertions raise an eyebrow. After looking I'm assuming they are due to a change in the BouncyCastle version for 3.5?

@kdubb
Copy link
Contributor

kdubb commented Oct 11, 2023

This ups bctls... quarkusio/quarkus#36331

@vsevel
Copy link
Contributor Author

vsevel commented Oct 11, 2023

same. no idea. does this ring a bell @geoand ?

@geoand
Copy link
Contributor

geoand commented Oct 11, 2023

None whatsoever

@kdubb
Copy link
Contributor

kdubb commented Oct 11, 2023

@vsevel In your local can you pin bctls back and see if that's the cause? Or maybe I'm making too much of it.

@vsevel
Copy link
Contributor Author

vsevel commented Oct 11, 2023

I don't see bctls being used while running the tests.
you added the module @kdubb
would that break something on your end? woud you consider it a regression?

@kdubb
Copy link
Contributor

kdubb commented Oct 11, 2023

It seems to be from ASN1Encodable.toString which isn't our concern. Just give me a second to check it.

@vsevel
Copy link
Contributor Author

vsevel commented Oct 11, 2023

certificate.getExtension(subjectAlternativeName).getParsedValue() prints [[CONTEXT 0][1.3.6.1.4.1.311.20.2.3, [CONTEXT 0]test], [CONTEXT 2]#616c742e6578616d706c652e636f6d, [CONTEXT 7]#01020304, [CONTEXT 6]#65783a3132333435]

@vsevel
Copy link
Contributor Author

vsevel commented Oct 11, 2023

when I decode the generated certificate with https://certlogik.com/decoder/ I get SAN

            X509v3 Subject Alternative Name: 
                othername:<unsupported>, DNS:alt.example.com, IP Address:1.2.3.4, URI:ex:12345

@kdubb
Copy link
Contributor

kdubb commented Oct 11, 2023

So the issue I'm concerned with is instability in our dependencies. After tracing them and debugging I'm seeing the BouncyCastle come from testcontainers. We should be getting it from Quarkus, no?

@vsevel
Copy link
Contributor Author

vsevel commented Oct 11, 2023

yes ASN1Util shaded from testcontainers

    public static String getTagText(int var0, int var1) {
        switch (var0) {
            case 64:
                return "[APPLICATION " + var1 + "]";
            case 128:
                return "[CONTEXT " + var1 + "]";

I had no idea we were using bc to begin with.
obviously, ideally we woud use the same lib than in prod.

@kdubb
Copy link
Contributor

kdubb commented Oct 11, 2023

Admittedly this is my mistake as I didn't check the provenance of the bouncy castle dependency and we are only using it in the test, given that maybe just go with it.

I'll open an issue to align BC with Quarkus.

@vsevel vsevel force-pushed the feature/quarkus_3.5 branch from 7fffecf to 26b2980 Compare October 11, 2023 18:44
@kdubb
Copy link
Contributor

kdubb commented Oct 11, 2023

@vsevel Nice! Does that auto-align from quarkus via BOM?

@vsevel
Copy link
Contributor Author

vsevel commented Oct 11, 2023

I have added this dep - this is coming from jdk18on-1.76:

        <dependency>
            <groupId>org.bouncycastle</groupId>
            <artifactId>bcprov-jdk18on</artifactId>
            <scope>test</scope>
        </dependency>
        <dependency>
            <groupId>org.bouncycastle</groupId>
            <artifactId>bcpkix-jdk18on</artifactId>
            <scope>test</scope>
        </dependency>

and corrected the imports.
it does not change the new behavior.

@kdubb
Copy link
Contributor

kdubb commented Oct 11, 2023

it does not change the new behavior.

Fine as long as we're aligned!

@vsevel
Copy link
Contributor Author

vsevel commented Oct 11, 2023

that was me doing the last upgrade back then (9 months ago)!
quarkusio/quarkus@288a8bd

I do not know why we have the change in behavior only now...

@vsevel
Copy link
Contributor Author

vsevel commented Oct 11, 2023

ok let's wait for the build. you validate?

@kdubb
Copy link
Contributor

kdubb commented Oct 11, 2023

👍

@vsevel vsevel merged commit 5bbfa83 into quarkiverse:main Oct 11, 2023
1 check passed
@vsevel vsevel deleted the feature/quarkus_3.5 branch October 11, 2023 18:59
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.

3 participants