Skip to content

Commit

Permalink
Merge branch 'master' of https://github.com/jenkinsci/jep into 229
Browse files Browse the repository at this point in the history
  • Loading branch information
jglick committed Nov 9, 2020
2 parents a8b9b6c + f52f17b commit 81176bd
Show file tree
Hide file tree
Showing 4 changed files with 28 additions and 22 deletions.
24 changes: 18 additions & 6 deletions jep/227/README.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ This work consists of several aspects, both in Jenkins core and plugins.
* Plugin-facing APIs referring to Acegi Security are kept binary compatible wherever feasible.
** Many `org.acegisecurity.*` types are reintroduced, but using clean reimplementations.
** Most existing Jenkins API methods referring to `org.acegisecurity.*` types are retained,
but deprecated and bridged to new Spring Security equivalents.
but deprecated and bridged to new Spring Security equivalents, typically ending in a `2` suffix.
** `toSpring` and `fromSpring` methods are provided to interconvert Acegi Security and Spring Security types where applicable.
** Subtyping is not used: the Acegi Security and Spring Security types are incomparable.
** `org.springframework.dao.DataAccessException` and similar types are deprecated without replacement.
Expand Down Expand Up @@ -92,7 +92,7 @@ In mostly chronological order, though of course many tasks can be parallelized:
** `ldap` needs extensive work.
* Run acceptance tests (ATH) and plugin compatibility tests (PCT) against core plus a representative subset of plugins.
* Interactively verify that core plus all plugins mentioned in the setup wizard (even if not `suggested`) seem to work.
* Evaluate the link:https://ci.jenkins.io/job/Core/job/jenkins/job/PR-4848/API_20compatibility/japicmp.html[japicmp report for Jenkins core] to make sure that all incompatible as well as compatible API changes are expected.
* Evaluate the japicmp report for Jenkins core to make sure that all incompatible as well as compatible API changes are expected.
* Solicit code reviews on all open associated pull requests to core and plugins.
* Create a compatibility chart associated with this JEP listing all known plugins that might be affected by this change, with their current status.
* Define a Jira label for regressions suspected to be related to this migration,
Expand Down Expand Up @@ -174,6 +174,22 @@ This seemed very confusing as we would continue to have two related APIs in the
It was also unclear how to make Jenkins implementation classes such as security filters work with such façades:
these classes dive heavily into details of the Acegi/Spring Security APIs, so would need numerous Acegi Security types to delegate, even if no plugin ever cared.

=== Java overloads vs. `2` suffix

In certain cases, a Spring version of a method could have been defined as a Java overload.
For example, `AccessControlled.hasPermission(Authentication, Permission)` could have had two overloads,
one for `org.acegisecurity.Authentication` and one for `org.springframework.security.core.Authentication`.

However in many cases the method changed _return_ type, which Java overloads do not support, meaning a new method name was required.
The convention in Jenkins APIs is to append `2` to replacement interfaces or methods (or `3` after `2`, etc.) so that was adopted here,
and for consistency was used in all cases even where an overload could have been used.

Thus a plugin developer moving to a post-Spring Jenkins baseline has a straightforward rule for most of the changes:
replace Acegi Security with Spring Security in `import` statements,
and append `2` to method calls or overrides where required to satisfy the compiler.
(There are a few other common changes which do not fit into this pattern, according to design changes in Spring Security,
such as `GrantedAuthority[]` changing to `Collection<? extends GrantedAuthority>`.)

=== Making Acegi Security types extend Spring Security types

Early attempt to bridge Acegi Security types to Spring Security types involved using subtype relationships.
Expand Down Expand Up @@ -383,8 +399,6 @@ producing a much shorter report.
Some matches are from plugins which already have preparatory patches.
A number of the remaining matches are Spring types that are _probably_ compatible from 2.x to 5.x.

You can also check the link:https://ci.jenkins.io/job/Core/job/jenkins/job/PR-4848/API_20compatibility/japicmp.html[japicmp report for Jenkins core].

== Security

This JEP changes Jenkins code fundamental to security and so introduces inherent security risks.
Expand Down Expand Up @@ -420,8 +434,6 @@ CloudBees is running the ATH & PCT against patched Jenkins core and many popular
** link:https://github.com/jenkinsci/jenkins/pull/4848[jenkins #4848] (upgrade to Spring Security 5 by jglick)
* Tracking
** link:compatibility.adoc[Compatibility table]
* Generated reports
** link:https://ci.jenkins.io/job/Core/job/jenkins/job/PR-4848/API_20compatibility/japicmp.html[japicmp report for Jenkins core]
* Searching for usages of Acegi Security in plugins
** link:https://github.com/jenkins-infra/usage-in-plugins/pull/15[usage-in-plugins #15] (PoC by Wadeck)
** link:https://github.com/jenkins-infra/usage-in-plugins/pull/16[usage-in-plugins #16] (general improvement by jglick)
Expand Down
17 changes: 3 additions & 14 deletions jep/227/compatibility.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -251,21 +251,10 @@ should be adjusted (was not specifically tested).
|Refers to `HudsonPrivateSecurityRealm` but is probably safe.

|link:https://plugins.jenkins.io/reverse-proxy-auth-plugin/[reverse-proxy-auth-plugin]
|Incompatible, fixes prepared
|link:https://github.com/jenkinsci/reverse-proxy-auth-plugin/pull/40[reverse-proxy-auth-plugin #40] may suffice; untested.

link:https://github.com/jenkinsci/reverse-proxy-auth-plugin/pull/38[reverse-proxy-auth-plugin #38]
is merely cleanup to make it easier to even test against new cores.

link:https://github.com/jenkinsci/reverse-proxy-auth-plugin/pull/37[reverse-proxy-auth-plugin #37]
would permit the LDAP-specific portions to be split into its own extension point and perhaps even plugin,
which could allow the core portion to be made compatible with Spring Security more easily.
|Probably compatible
|As of link:https://github.com/jenkinsci/reverse-proxy-auth-plugin/releases/tag/reverse-proxy-auth-plugin-1.7.0[1.7.0].

The plugin has not been released in 2½ years;
even then it was only released by Jenkins CERT members
to allow critical fixes for link:../200/README.adoc[JEP-200]
(link:https://groups.google.com/g/jenkinsci-dev/c/9mX-S7kLnHk/m/J6tlhM6vAQAJ[background]).
It appears to now be abandoned and in need of adoption.
This plugin is in need of adoption.

|link:https://plugins.jenkins.io/saml/[saml]
|Compatible
Expand Down
2 changes: 2 additions & 0 deletions jep/228/README.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@ If link:../7/README.adoc[JEP-7] is implemented, then `XStream2.setMapper` and `.

Maintaining a custom fork of a library was an additional maintenance burden for Jenkins developers.
The fork was well out of date, leading to suspicions that security hardenings added in recent years may be missing, though no vulnerabilities are known.
Jenkins was also missing out on potentially useful improvements, such as
link:https://github.com/x-stream/xstream/issues/101[better Java 11 compatibility].

A key reason for maintaining the fork was to introduce thread safety as well as concurrency-optimized collections into XStream.
In fact upstream XStream is designed to be thread-safe _after_ its configuration has been frozen.
Expand Down
7 changes: 5 additions & 2 deletions jep/229/README.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,6 @@ As of this writing unimplemented:

To avoid the need for each component maintainer to manually create a bot account and perform similar setup,
tooling at the organization level should automate this aspect.
(As of this writing unimplemented.)

link:https://github.com/jenkins-infra/repository-permissions-updater/blob/6ff61dc11b830f1984dde4ba9e82870218d702f7/Jenkinsfile#L54-L56[Repository Permissions Updater (RPU) processing]
is patched to recognize some flag in a repo’s YAML entry (e.g., `bot: true`)
Expand Down Expand Up @@ -355,7 +354,11 @@ since there is no immediate user impact of a new release appearing of such a com

== Prototype Implementation

The link:https://github.com/jenkinsci/log-cli-plugin[`log-cli` plugin] implements basic aspects of this proposal.
The link:https://github.com/jenkinsci/log-cli-plugin[`log-cli` plugin]
implements basic aspects of this proposal from the developer side.

link:https://github.com/jenkins-infra/repository-permissions-updater/pull/1747[repository-permissions-updater #1747]
implements changes to infrastructure.

== References

Expand Down

0 comments on commit 81176bd

Please sign in to comment.