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

Add org.opensaml.security to X-Pack Security module-info #102589

Merged
merged 1 commit into from
Dec 7, 2023

Conversation

droberts195
Copy link
Contributor

My IntelliJ was refusing to compile SamlObjectHandler.java unless this extra line was added to module-info.java.

This doesn't currently affect official builds, nor anyone else's IntelliJ builds it would seem. However, I thought that since eventually some other change will make this necessary it doesn't hurt to add it now, and might save others time.

My IntelliJ was refusing to compile SamlObjectHandler.java
unless this extra line was added to module-info.java.

This doesn't currently affect official builds, nor anyone
else's IntelliJ builds it would seem. However, I thought
that since eventually some other change will make this
necessary it doesn't hurt to add it now, and might save
others time.
@droberts195 droberts195 added >non-issue :Security/Security Security issues without another label v8.12.0 labels Nov 24, 2023
@elasticsearchmachine elasticsearchmachine added the Team:Security Meta label for security team label Nov 24, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

@droberts195
Copy link
Contributor Author

This was the error I was getting:

Screenshot 2023-11-24 at 11 37 01

The specific lines that were red in the IDE were these ones:

import org.opensaml.security.credential.Credential;
import org.opensaml.security.x509.X509Credential;

@breskeby breskeby requested review from ChrisHegarty and breskeby and removed request for breskeby November 24, 2023 13:19
@ChrisHegarty
Copy link
Contributor

How things are setup here has changed fairly recently; use to be a shadow jar, now a transformed artifact, from #98199.

The relevant imports in SamlObjectHandler are :

import org.opensaml.security.credential.Credential;
import org.opensaml.security.x509.X509Credential;

The packages containing these classes come from "org.opensaml:opensaml-security-api", which is transformed in the build, see x-pack/libs/es-opensaml-security-api. The dependency from x-pack/security is on this transformed artifect. The transformed artifect has an Automatic-Module-Name: org.opensaml.security. This all seems ok.

The requires org.opensaml.security (as proposed in this PR) makes sense. But with it now my IDEA ( version 2023.2.1 ) complains:
Screenshot 2023-11-24 at 14 50 35

IDEA is having problems with this transformed artifact, or that the dependency is from a shadow configuration:

  api project(path: ':x-pack:libs:es-opensaml-security-api', configuration: 'shadow')

@albertzaharovits
Copy link
Contributor

IDEA is having problems with this transformed artifact, or that the dependency is from a shadow configuration:

I think that's the main problem and the module issue is just a symptom. I use the latest IntelliJ 2023.2.5.

I can see the same missing imports in the SamlFactory class from the x-pack.plugin.identity-provider project, which doesn't use modules (the imports are from the shadowed jar).
I checked and we already use the latest version of the shadow-jar plugin 8.1.1 (

shadow-plugin = "com.github.johnrengelman:shadow:8.1.1"
https://imperceptiblethoughts.com/shadow/getting-started/).

@albertzaharovits
Copy link
Contributor

I think I erroneously removed the module require statement in https://github.com/elastic/elasticsearch/pull/101904/files#r1394808215 (because my IntelliJ version was already not recognizing the shadowed jar deps).

@albertzaharovits
Copy link
Contributor

I think we should merge this PR, and deal with the unrecognized shadow deps separately.

Copy link
Contributor

@arteam arteam left a comment

Choose a reason for hiding this comment

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

LGTM

@droberts195 droberts195 merged commit 1913145 into elastic:main Dec 7, 2023
@droberts195 droberts195 deleted the fix_security_module_info branch December 7, 2023 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>non-issue :Security/Security Security issues without another label Team:Security Meta label for security team v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants