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.

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

Jump to bottom

UNDERTOW-1881 - Add a new exchange attribute for SSL/TLS protocol version #1707

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

UNDERTOW-1881 - Add a new exchange attribute for SSL/TLS protocol version #1707

wants to merge 1 commit into from

Conversation

jasondlee
Copy link
Contributor

https://issues.redhat.com/browse/UNDERTOW-1881

Add and register new ExchangeAttribute implementation

@jasondlee jasondlee requested a review from fl4via November 21, 2024 16:33
@jasondlee jasondlee requested a review from ropalka November 21, 2024 20:20
@baranowb
Copy link
Contributor

Shouldnt this be aiming towards main?

@jasondlee
Copy link
Contributor Author

Shouldnt this be aiming towards main?

Perhaps. I couldn't remember where they're supposed to go, but I think you're right. We target main, and @fl4via backports as needed? I'll update the target...

@jasondlee jasondlee marked this pull request as ready for review November 25, 2024 16:54
@jasondlee jasondlee changed the base branch from 2.3.x to main November 25, 2024 16:54
@jasondlee jasondlee changed the base branch from main to 2.3.x November 25, 2024 16:58
@jasondlee jasondlee changed the base branch from 2.3.x to main November 25, 2024 16:59
@baranowb baranowb added under verification Currently being verified (running tests, reviewing) before posting a review to contributor new feature/API change New feature to be introduced or a change to the API (non suitable to minor releases) waiting peer review PRs that edit core classes might require an extra review labels Nov 26, 2024
@baranowb baranowb self-requested a review November 27, 2024 07:07
baranowb
baranowb reviewed Nov 27, 2024
View reviewed changes
Copy link
Contributor

@baranowb baranowb left a comment

Choose a reason for hiding this comment

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

As is its fine, however given comment from @msfm : https://issues.redhat.com/browse/UNDERTOW-1881?focusedId=16067599&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16067599
Im not 100% sure this will work in all cases?

@jasondlee jasondlee requested a review from baranowb December 3, 2024 22:56
@jasondlee jasondlee force-pushed the UNDERTOW-1881 branch 4 times, most recently from 9ce8420 to f8e3988 Compare December 4, 2024 02:25
baranowb
baranowb requested changes Dec 5, 2024
View reviewed changes
core/src/main/java/io/undertow/server/handlers/accesslog/DefaultAccessLogReceiver.java
@@ -234,7 +234,7 @@ public void run() {
* <p>
* DO NOT USE THIS OUTSIDE OF A TEST
*/
void awaitWrittenForTest() throws InterruptedException {
protected void awaitWrittenForTest() throws InterruptedException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Im not keen on this. Maybe just ping pong request and set special header to attribute value?
https://github.com/undertow-io/undertow/blob/main/core/src/test/java/io/undertow/server/handlers/SetAttributeTestCase.java

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you clarify what you mean here?

Copy link
Contributor

@baranowb baranowb Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Is this var used in any place

undertow/core/src/test/java/io/undertow/server/ssl/SecureProtocolAttributeTestCase.java

Line 51 in 065c6d0

LocalAccessLogReceiver logReceiver
?

Copy link
Contributor

Choose a reason for hiding this comment

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

Above ^

core/src/main/java/io/undertow/attribute/SslProtocolAttribute.java Outdated
String sslProtocol = null;
String transportProtocol = exchange.getConnection().getTransportProtocol();
if ("ajp".equals(transportProtocol)) {
// TODO: wrong
Copy link
Contributor

Choose a reason for hiding this comment

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

Whats the correct way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH, I'm not sure. I left that there as a reminder after reading a comment from Masafumi, iirc. I'll ping him.

@jasondlee
[UNDERTOW-1881] - Add a new exchange attribute for SSL/TLS protocol v…
065c6d0 
…ersion

Add and register new ExchangeAttribute implementation
Add support for AJP and TLS
Add system property to help identify proxied tests
Add test case
@jasondlee jasondlee requested a review from baranowb January 13, 2025 20:49
baranowb
baranowb requested changes Jan 20, 2025
View reviewed changes
core/src/main/java/io/undertow/server/handlers/accesslog/DefaultAccessLogReceiver.java
@@ -234,7 +234,7 @@ public void run() {
* <p>
* DO NOT USE THIS OUTSIDE OF A TEST
*/
void awaitWrittenForTest() throws InterruptedException {
protected void awaitWrittenForTest() throws InterruptedException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Above ^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@baranowb baranowb baranowb requested changes

@fl4via fl4via Awaiting requested review from fl4via

@ropalka ropalka Awaiting requested review from ropalka

Assignees
No one assigned
Labels
new feature/API change New feature to be introduced or a change to the API (non suitable to minor releases) under verification Currently being verified (running tests, reviewing) before posting a review to contributor waiting peer review PRs that edit core classes might require an extra review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jasondlee @baranowb