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

refactoring ProtocolHelper #30657

Open
wants to merge 2 commits into
base: integration
Choose a base branch
from

Conversation

jacobwdv
Copy link
Contributor

@jacobwdv jacobwdv commented Jan 28, 2025

Resolves #30583

@jacobwdv jacobwdv self-assigned this Jan 28, 2025
@jacobwdv
Copy link
Contributor Author

Looking at the code before, it doesn't make sense to me that we handled single item lists differently than lists with multiple items.

It seems the previous code would use a cache only if multi-protocol is specified. That would cause performance slowdown on single-protocol lists since it would execute checkProtocol() everytime checkProtocolValueGood() was called.

This change uses a cache for any sized list.

It simplifies the loop which validates the protocol by using the same loop regardless of FIPS being enabled or not

Lastly, we could simplify this even farther by checking the cache before we check allowedProtocols. I didn't make this change because it would break in the case that the user enabled fips without restarting their server.

            for (String protocol : protocols) {
                if (allowedProtocols.contains(protocol)) {
                    if (validatedProtocols.contains(protocol))
                        continue;
                    else {
                        checkProtocol(protocol);
                        validatedProtocols.add(protocol);
                    }
                } else {
                    Tr.error(tc, "ssl.protocol.error.CWPKI0832E", protocol);
                    throw new SSLException("Protocol provided is not appropriate for a protocol list.");
                }
            }

to

        if (protocols.length > 0) {
            for (String protocol : protocols) {
                if (validatedProtocols.contains(protocol))
                    continue;
                else if (allowedProtocols.contains(protocol)) {
                    checkProtocol(protocol);
                    validatedProtocols.add(protocol);
                } else {
                    Tr.error(tc, "ssl.protocol.error.CWPKI0832E", protocol);
                    throw new SSLException("Protocol provided is not appropriate for a protocol list.");
                }
            }
        }

@jacobwdv jacobwdv force-pushed the JD_30583 branch 2 times, most recently from 5b5666b to 133d49c Compare January 28, 2025 19:51
@jacobwdv
Copy link
Contributor Author

jacobwdv commented Jan 28, 2025

#build (view Open Liberty Personal Build - ❌ completed with errors/failures.)

Note: Target locations of links might be accessible only to IBM employees.

@LibbyBot
Copy link

Code analysis and actions

DO NOT DELETE THIS COMMENT.
  • 1 product code files were changed.
  • Please describe in a separate comment how you tested your changes.

@LibbyBot
Copy link

Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_SDtiwN4MEe-M5OZShUU9WA

Target locations of links might be accessible only to IBM employees.

Copy link
Member

@jhanders34 jhanders34 left a comment

Choose a reason for hiding this comment

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

This is a slight behavior change that could break customers. In the protocol length of 1 scenario, we allowed any protocol even if it wasn't in the MULTI_PROTOCOL_LIST list. It appears that in the > 1 scenario we care about the list not containing certain protocols. To not break existing customers, I believe we would have to have logic for 140-3 to check the FIPS_140_3_PROTOCOLS whether providing 1 or more than 1 protocol, but for the 1 protocol, we wouldn't do a check against allowedProtocols if not using 140-3.

@jacobwdv jacobwdv requested a review from jhanders34 January 29, 2025 19:27
@jacobwdv
Copy link
Contributor Author

jacobwdv commented Jan 29, 2025

#build (view Open Liberty Personal Build - ❌ completed with errors/failures.)

Note: Target locations of links might be accessible only to IBM employees.

@LibbyBot
Copy link

Code analysis and actions

DO NOT DELETE THIS COMMENT.
  • 1 product code files were changed.
  • Please describe in a separate comment how you tested your changes.

@LibbyBot
Copy link

Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_Oa2ykd8FEe-M5OZShUU9WA

Target locations of links might be accessible only to IBM employees.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants