[JEP-227] Replace Acegi Security with Spring Security & upgrade Spring Framework #4848

jglick · 2020-07-15T23:03:29Z

JEP-227

Proposed changelog entries

Jenkins now uses Spring Security rather than its predecessor, Acegi Security. Other bundled Spring libraries are also updated.

Proposed upgrade guidelines

https://www.jenkins.io/blog/2020/11/10/spring-xstream/

See the compatibility table.

Desired reviewers

@jenkinsci/core

Maintainer checklist

Before the changes are marked as ready-for-merge:

There are at least 2 approvals for the pull request and no outstanding requests for change
Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
Changelog entries in the PR title and/or Proposed changelog entries are correct
Proper changelog labels are set so that the changelog can be generated automatically
If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

timja

looks like a good approach

…st-harness

core/src/main/java/hudson/ExpressionFactory2.java

jglick · 2020-07-21T11:18:05Z

core/src/main/java/hudson/ExtensionFinder.java

@@ -578,7 +578,7 @@ public Object get() {
                // so that we invoke them before derived class one. This isn't specified in JSR-250 but implemented
                // this way in Spring and what most developers would expect to happen.

-                final Set<Class> interfaces = ClassUtils.getAllInterfacesAsSet(instance);
+                final Set<Class<?>> interfaces = ClassUtils.getAllInterfacesAsSet(instance);


Just a signature change in Spring.

jglick · 2020-07-21T11:21:17Z

core/src/main/java/hudson/security/AbstractPasswordBasedSecurityRealm.java

+        // we just authenticate anonymous users as such,
+        // so that later authorization can reject them if so configured
+        AnonymousAuthenticationProvider aap = new AnonymousAuthenticationProvider("anonymous");
+        AuthenticationManager authenticationManager = new ProviderManager(authenticator, rmap, aap);


Just translating the Groovy into equivalent Java.

core/src/main/java/hudson/security/AccessDeniedException2.java

core/src/main/java/hudson/security/HttpSessionContextIntegrationFilter2.java

core/src/main/java/org/acegisecurity/AuthenticationManager.java

jglick · 2020-07-21T11:43:21Z

core/src/main/java/org/acegisecurity/acls/sid/PrincipalSid.java

+import org.springframework.security.core.Authentication;
+import org.springframework.security.core.userdetails.UserDetails;
+
+public class PrincipalSid implements Sid {


Reimplementing these from scratch. Only used from SidAcl.

and cloudbees/bluesteel -> TeamSecurity.java (which is not using SidACL in that class but underlying more it it is). (not sure you saw that or not)

Ought to be compatible AFAICT.

jglick · 2020-07-21T11:44:34Z

core/src/main/java/org/acegisecurity/providers/ldap/authenticator/BindAuthenticator2.java

- * 
- * @author Kohsuke Kawaguchi
- */
-public class BindAuthenticator2 extends BindAuthenticator {


Seems to have been unused, and I do not want to depend on Spring Security modules for LDAP here; belongs in the ldap plugin only.

Correction: was used by the ldap plugin, and should have been there all along (see 9b4cd99 & 32cba8f). Correcting in jenkinsci/ldap-plugin@a5267e6.

jglick · 2020-07-21T11:44:58Z

core/src/main/java/org/acegisecurity/userdetails/UserDetailsService.java

+ * @deprecated TODO replacement
+ */
+@Deprecated
+public interface UserDetailsService extends org.springframework.security.core.userdetails.UserDetailsService {}


Might just get deleted, TBD.

core/src/main/java/org/acegisecurity/AuthenticationException.java

…k me a while to find that HttpSessionContextIntegrationFilter is now SecurityContextPersistenceFilter

jvz · 2020-11-04T15:47:12Z

I'll do one last review today, @timja.

jvz

LGTM. Tested this as part of the compatibility testing efforts on various plugins. We already discussed the pervasive 2 suffix and how it should be helpful for developers to update their API usages.

jvz · 2020-11-04T16:23:18Z

core/src/main/java/hudson/security/AuthenticationProcessingFilter2.java

        LOGGER.log(Level.FINE, "Login attempt failed", failed);
+        /* TODO this information appears to have been deliberately removed from Spring Security:
        Authentication auth = failed.getAuthentication();
        if (auth != null) {
            SecurityListener.fireFailedToLogIn(auth.getName());


I'd kind of like to get more data exposed in audit-log-plugin, though that doesn't appear to listen for this event yet. It is listening for login and logout events, though, so it would make sense to support.

jvz · 2020-11-04T16:28:18Z

core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java

-        }
-    };
-
+    // TODO can we instead use BCryptPasswordEncoder from Spring Security, which has its own copy of BCrypt so we could drop the special library?


I think so. BouncyCastle provides BCrypt, too, so it's not like a separate library was needed in the first place.

We cannot use BouncyCastle from core, though.

I suppose it shouldn't matter for this case, then, since Spring Security already includes a BCrypt implementation in its own dependencies like you mentioned in the comment here. Does this limitation include for remoting? That'd be annoying if the only way to support TLS 1.3 connections there is to use Java 11 or customizing your classpath manually.

Not sure what you are talking about. BouncyCastle is bundled only in a wrapper plugin for use by other plugins.

BouncyCastle has JSSE glue to support TLS 1.3 in older JDKs than 11. Classes like SSLContext call into that which is the usual class for handling TLS code in Java.

Sorry, do not know anything about it. Off topic I suppose. The point here is that for now I kept the existing behavior of using our own bundled jBCrypt, but now that Spring Security also bundles their own copy, we could consider switching to that and dropping our dep. I did not attempt to do so here because the format uses by BCryptPasswordEncoder does not match that of existing saved passwords in Jenkins, so we would need to do more work.

jvz · 2020-11-04T16:32:01Z

core/src/main/java/hudson/security/SidACL.java

+import java.util.logging.Logger;
+import org.acegisecurity.acls.sid.GrantedAuthoritySid;
+import org.acegisecurity.acls.sid.PrincipalSid;
+import org.acegisecurity.acls.sid.Sid;


Should spring-security-acl be blocked as a dependency then?

daniel-beck · 2020-11-06T16:19:23Z

Jenkinsfile

@@ -69,6 +69,7 @@ for(j = 0; j < jdks.size(); j++) {
                                             allowEmptyArchive: true, // in case we forgot to reincrementalify
                                             fingerprint: true
                        }
+                        publishHTML([allowMissing: true, alwaysLinkToLastBuild: false, includes: 'japicmp.html', keepAll: false, reportDir: 'core/target/japicmp', reportFiles: 'japicmp.html', reportName: 'API compatibility', reportTitles: 'japicmp report'])


Do we want to merge this in?

⇒ #4848 (comment)

daniel-beck · 2020-11-06T16:27:31Z

core/pom.xml

+          <plugin>
+            <groupId>com.github.siom79.japicmp</groupId>
+            <artifactId>japicmp-maven-plugin</artifactId>
+            <version>0.14.4-20200728.214757-1</version> <!-- TODO https://github.com/siom79/japicmp/pull/266 -->


Similarly doesn't look like something we want to merge in.

I think it is OK because this is only used in a profile we active in CI, so if my PR never gets picked up we can either drop this profile, or use the last mojo release which will work for most purposes (just not complex library replacements like is done here).

Happy to remove it if you are uneasy.

I'm fine with leaving it. I actually ran the profile and examined the results. If Daniel is concerned, though, I readily accept the removal.

…ns#4944 to be as well, to be released 2020-11-10

jglick · 2020-12-09T18:19:47Z

core/src/main/java/org/acegisecurity/util/FieldUtils.java

+
+    public static void setProtectedFieldValue(String protectedField, Object object, Object newValue) {
+        try {
+            org.apache.commons.lang.reflect.FieldUtils.writeField(object, protectedField, newValue, true);


jtnord · 2021-01-29T13:41:02Z

core/src/main/java/hudson/model/UpdateCenter.java

@@ -67,7 +67,6 @@
 import jenkins.util.io.OnMaster;
 import net.sf.json.JSONObject;

-import org.acegisecurity.Authentication;


noting regression https://issues.jenkins-ci.org/browse/JENKINS-64746 addressed in #5216

timja · 2021-03-22T08:55:12Z

@jglick / @jvz looks like the credentials API needs updating? https://github.com/jenkinsci/credentials-plugin/blob/3a603dec20412ccce754d9cbbd835d890ca06ee5/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java#L517

It takes the acegi security Authentication as a parameter and I can't avoid a deprecation warning in my plugin because of this.

jglick · 2021-03-22T11:42:23Z

It could be updated, sure.

This is the implementation of jenkinsci/jenkins#4848 for Credentials API. This will allow consumers of the credentials API to remove references to deprecated acegi APIs.

…APIs (#490) * [JEP-227] Replace Acegi Security with Spring Security APIs This is the implementation of jenkinsci/jenkins#4848 for Credentials API. This will allow consumers of the credentials API to remove references to deprecated acegi APIs. * Fix spotbugs issues * Fix reviews * Rename methods appropriately * Fixup some javadocs. * Remove unused method. * Forgot to remove usages. * Remove CredentialsProvider#getCredentials2(Class, ItemGroup, Authentication) in favor of CredentialsProvider#getCredentials2(Class, ItemGroup, Authentication, List) * Fix a few null checks while we are here. * Restore a method I deleted by mistake. * Add tests for new signatures * Rename *2 methods to *InItem/*InItemGroup to avoid ambiguous signatures * Update docs

Replacing Acegi Security with Spring Security

2caf075

jglick requested review from daniel-beck and Wadeck July 15, 2020 23:03

more

82522cf

timja reviewed Jul 18, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

jglick added 2 commits July 19, 2020 10:22

Change of approach: completely deprecating org.acegisecurity types

62eba9c

More conversion

0c02c44

res0nance added the work-in-progress The PR is under active development, not ready to the final review label Jul 20, 2020

jglick added 9 commits July 20, 2020 16:14

Merge branch 'master' of github.com:jenkinsci/jenkins into spring-5303

d898b35

More

f573052

Progress

07412a9

Merge branch 'master' of github.com:jenkinsci/jenkins into spring-5303

0613a19

Seems that everything except uses of BeanBuilder now compiles

6af8f1c

Rewrite BeanBuilder configuration of security to plain old Java

9111057

core module now at least compiles

6601d98

Starting to make tests compilable; will require changes to jenkins-te…

b85c0d9

…st-harness

Tweaking AuthenticationProcessingFilter2

cfd9831

jglick commented Jul 21, 2020

View reviewed changes

jglick added 11 commits July 21, 2020 07:48

Comments

085f525

Merge branch 'master' of github.com:jenkinsci/jenkins into spring-5303

26060aa

Merge branch 'support-log-formatter' into spring-5303

c6f0e12

Better logging for ChainedServletFilter

c3f9efd

Can now log in; crumbs are broken though

a8a661e

Searched more extensively for references to Acegi Security

76b0885

HttpSessionContextIntegrationFilter2 was critical after all; just too…

12598a7

…k me a while to find that HttpSessionContextIntegrationFilter is now SecurityContextPersistenceFilter

Comment

a985a50

Fixing UserSeedProperty so that login works at last

4201baa

Binary compatibility for QueueItemAuthenticator

a36f410

Fixing up QueueItemAuthenticator Javadoc

ae31edd

jvz approved these changes Nov 4, 2020

View reviewed changes

daniel-beck reviewed Nov 6, 2020

View reviewed changes

jglick mentioned this pull request Nov 6, 2020

[JEP-227] Formalizing discussion about 2 suffix jenkinsci/jep#329

Merged

jvz mentioned this pull request Nov 6, 2020

[JEP-228] Unforking XStream #4944

Merged

6 tasks

jeffret-b merged commit a9ca5ef into jenkinsci:master Nov 6, 2020

jglick deleted the spring-5303 branch November 6, 2020 18:33

jglick mentioned this pull request Nov 6, 2020

[JEP-227] Links to japicmp report will be dead jenkinsci/jep#331

Merged

jglick added a commit to jglick/jenkins.io that referenced this pull request Nov 6, 2020

jenkinsci/jenkins#4848 merged toward 2.266, expecting jenkinsci/jenki…

17cc0cf

…ns#4944 to be as well, to be released 2020-11-10

jglick commented Dec 9, 2020

View reviewed changes

jglick mentioned this pull request Jan 21, 2021

Modernize plugin, update to 2.277.x+ jenkinsci/matrix-auth-plugin#97

Merged

escoem mentioned this pull request Jan 29, 2021

[JENKINS-64746] public API hudson.model.UpdateCenter.HudsonUpgradeJob constructor signature change #5216

Merged

10 tasks

jtnord reviewed Jan 29, 2021

View reviewed changes

daniel-beck mentioned this pull request Feb 9, 2021

Add at-since annotations up to 2.279 #5276

Merged

10 tasks

inakoll mentioned this pull request Aug 31, 2021

Replace Acegi Security with Spring Security & upgrade Spring Framework jenkinsci/mixing-security-realm-plugin#2

Open

6 tasks

jglick mentioned this pull request Jan 14, 2022

Jakarta EE 9 transition #6085

Closed

basil mentioned this pull request Mar 13, 2022

Remove japicmp #6367

Merged

11 tasks

jglick mentioned this pull request Aug 4, 2022

Jakarta EE 9 transition jenkinsci/stapler#301

Closed

jglick mentioned this pull request Sep 18, 2023

[chore] Remove useless TODO #8501

Merged

jglick mentioned this pull request Oct 3, 2023

[JENKINS-72114] Fix serialization of HudsonPrivateSecurityRealm.UserDetailsImpl #8557

Merged

Vlatombe mentioned this pull request Oct 23, 2023

[JEP-227][JENKINS-39324] Replace Acegi Security with Spring Security APIs jenkinsci/credentials-plugin#490

Merged

jglick mentioned this pull request Dec 5, 2023

JEP-227 update for JTH jenkinsci/jenkins-test-harness#695

Merged

jglick mentioned this pull request Jan 29, 2024

Fixing NotSerializableException: org.acegisecurity.context.SecurityContext$1 #8918

Merged

jglick mentioned this pull request Feb 21, 2024

Assorted cleanups jenkinsci/cloudbees-folder-plugin#379

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JEP-227] Replace Acegi Security with Spring Security & upgrade Spring Framework #4848

[JEP-227] Replace Acegi Security with Spring Security & upgrade Spring Framework #4848

jglick commented Jul 15, 2020 •

edited

timja left a comment

This comment has been minimized.

jglick Jul 21, 2020

jglick Jul 21, 2020

jglick Jul 21, 2020

jtnord Sep 1, 2020

jglick Sep 1, 2020

jglick Jul 21, 2020

jglick Jul 29, 2020

jglick Jul 21, 2020

jvz commented Nov 4, 2020

jvz left a comment

jvz Nov 4, 2020

jvz Nov 4, 2020

jglick Nov 4, 2020

jvz Nov 4, 2020

jglick Nov 4, 2020

jvz Nov 4, 2020

jglick Nov 4, 2020

jvz Nov 4, 2020

daniel-beck Nov 6, 2020

jglick Nov 6, 2020

daniel-beck Nov 6, 2020

jglick Nov 6, 2020

jeffret-b Nov 6, 2020

jglick Dec 9, 2020

jtnord Jan 29, 2021

timja commented Mar 22, 2021

jglick commented Mar 22, 2021

[JEP-227] Replace Acegi Security with Spring Security & upgrade Spring Framework #4848

[JEP-227] Replace Acegi Security with Spring Security & upgrade Spring Framework #4848

Conversation

jglick commented Jul 15, 2020 • edited

Proposed changelog entries

Proposed upgrade guidelines

Desired reviewers

Maintainer checklist

timja left a comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvz commented Nov 4, 2020

jvz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timja commented Mar 22, 2021

jglick commented Mar 22, 2021

jglick commented Jul 15, 2020 •

edited