Saml http metadata resolver refactor

[MaciejMierzwa]

Description

Refactor SamlHTTPMetadataResolver to remove of HTTP commons 4 lib

• Category (Enhancement, Refactoring, Maintenance)
• Behavior should stay the same

Issues Resolved

• Resolves [Maintenance] Review use of SamlHTTPMetadataResolver #2932

Testing

No functionality changes, tests refactored a little.

Check List

• New functionality includes testing
• New functionality has been documented
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[codecov]

Codecov Report

Attention: 45 lines in your changes are missing coverage. Please review.

Comparison is base (b996eb1) 66.60% compared to head (31cb3e0) 65.08%.
Report is 25 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3894      +/-   ##
==========================================
- Coverage   66.60%   65.08%   -1.52%     
==========================================
  Files         298      299       +1     
  Lines       21188    21286      +98     
  Branches     3453     3463      +10     
==========================================
- Hits        14112    13855     -257     
- Misses       5361     5709     +348     
- Partials     1715     1722       +7     

Files	Coverage Δ
...zon/dlic/auth/http/saml/HTTPSamlAuthenticator.java	67.51% <ø> (ø)
...azon/dlic/util/SettingsBasedSSLConfiguratorV4.java	59.69% <40.00%> (+0.30%)	⬆️
.../dlic/auth/http/saml/SamlHTTPMetadataResolver.java	51.61% <14.28%> (-9.93%)	⬇️
...azon/dlic/auth/http/saml/HTTPMetadataResolver.java	45.45% <45.45%> (ø)

... and 47 files with indirect coverage changes

[willyborankin]

Thank you @MaciejMierzwa. Added some comments.

[peternied]

@MaciejMierzwa Can this be backported into 2.x - can you take a look if this is possible? I'm concerned that the Apache Http Client 5 will create some issues in that codebase

[cwperks]

@MaciejMierzwa With this PR can these 3 dependencies (https://github.com/opensearch-project/security/blob/main/build.gradle#L579-L581) be changed to integrationTestImplementation?

[MaciejMierzwa]

@peternied I think it can be backported into 2.x. Currently 2.x doesn't contain dependencies on any of http5 libraries. If the library is allowed I think it's little bit of additional work.

@cwperks question:
I mostly focused on SamlHTTPMetadataResolver class and the surrounding ones. There is still quite a bit of http4 used in implementation, mostly static headers and HTTP codes are imported from http4 library.

[peternied]

@peternied I think it can be backported into 2.x. Currently 2.x doesn't contain dependencies on any of http5 libraries. If the library is allowed I think it's little bit of additional work.

@MaciejMierzwa We can accept these changes in the main branch, we would be in striking distance to remove all http client 4 references. There might be other issue aside just importing the library [1], but I'd rather move forward with what you have, just want you to be aware the backport might be a little gross.

• [META] Breaking changes in 3.0 OpenSearch#3351

[MaciejMierzwa]

Hi I noticed that checkstyle fails for http5 library. Looks like this whole PR could be reversed
https://github.com/opensearch-project/security/pull/3741/files

[DarshitChanpura]

Hi I noticed that checkstyle fails for http5 library. Looks like this whole PR could be reversed
https://github.com/opensearch-project/security/pull/3741/files

If http 5 is necessary for this PR to resolve, we may need to revert back to http 5 but that would leave this bug unresolved. If the only reason we were not moving to http 5 is due to the saml metadata resolver, we can revert the mentioned PR to move back to http 5 since this PR migrates the resolver to http 5 as well.

[peternied]

Hi I noticed that checkstyle fails for http5 library. Looks like this whole PR could be reversed https://github.com/opensearch-project/security/pull/3741/files

The checkstyle issue is associated with the use of client5 specific status codes, you can use the broader ones

-import·org.apache.hc.core5.http.HttpStatus;
+import·org.apache.http.HttpStatus;

[DarshitChanpura]

The checkstyle issue is associated with the use of client5 specific status codes, you can use the broader ones

Curious, because the changes introduced here refactor the usage from http 4 to http 5 and I believe they are necessary for this PR. Correct me if I'm wrong here @MaciejMierzwa . If they are indeed required to be http5 what should be the path forward?

[DarshitChanpura]

@MaciejMierzwa seems like the progress on this PR has stalled. Do you still intend to work on this? Maintainers will be closing this PR out soon if there is no activity.

[MaciejMierzwa]

Hi @DarshitChanpura sorry, I see that I got mail notifications, must've missed it. Regarding comments from #3894 (comment) onward - I think the idea behind this ticket was to move towards removal of http4 library from the codebase. @cwperks also highlighted in this comment #3581 (comment) that http4 usage was due to opensaml library, causing conflicts.
For me, it looks like if this PR gets backported to 2.x the bug mentioned by Peter in #3581 get's resolved. What do you think?

[peternied]

I see code patterns around the logging that are unique and distinct from the security codebase, was this class's contents copied from another implementation?

@MaciejMierzwa We do not merge unattributed code copied from other code bases, this violates the DCO - developer certificate of origin. I am closing out this PR.