JENKINS-64629: Fix "null" in error message

[jvz]

@rsandell

https://issues.jenkins.io/browse/JENKINS-64629

Unfortunately, spotbugs does not appear to be sophisticated enough to have prevented this.

[basil]

In both JENKINS-64629 and JENKINS-64850, the DisabledException comes from line 92 of UserAttributesHelper#checkIfUserEnabled:

org.springframework.security.authentication.DisabledException: The user "null" is administratively disabled.
at hudson.security.UserAttributesHelper.checkIfUserEnabled(UserAttributesHelper.java:92)

The Messages.UserDetails_Disabled exception constructor takes as input an argument representing the user:

src/main/resources/jenkins/security/plugins/ldap/Messages.properties
63:UserDetails.Disabled=The user "{0}" is administratively disabled.

On line 92 of UserAttributesHelper#checkIfUserEnabled, we pass in user.get("dn") as this argument when creating Messages.UserDetails_Disabled:

throw new DisabledException(Messages.UserDetails_Disabled(user.get("dn")));

This implies that user was non-null, since we were able to successfully dereference it and call .get("dn"). Otherwise we would have received a NullPointerException, not a DisabledException. So I doubt this fix is correct. This fix is assuming that we called UserAttributesHelper#checkIfUserEnabled with a null user argument, but that couldn't have been the case if we were able to successfully dereference it to create the DisabledException that users reported. Feel free to let me know if I am misunderstanding something.

[jvz]

Alright, that starts to sound more like an end-user problem more so than a bug then. All LDAP objects have a dn; otherwise, they wouldn't be objects we could look up in the first place!

[jvz]

Based on that description, though, I have a better idea. Let me update this.

[basil]

Sorry, I do not have the context to be able to meaningfully review this change. You might want to try reproducing the user-reported issue in a test using FakeChangeLogSCM with AbstractBuild#getCulprits and User#impersonate (the two methods Mailer and Email Extension are calling in the user-reported issue), or else working with the users in JENKINS-64629 and JENKINS-64850 to have them install an incremental build to gather additional state that might assist in getting to a root cause.

[jglick]

Looks reasonable. Is there some known way to reproduce the error that could be added to a test?

[basil]

I think the more pressing question is the impact of JENKINS-55813 on the existing implementation of SECURITY-372 in Mailer and Email Extension. The logic added in SECURITY-372 (in both Mailer and Email Extension) assumes that User#impersonate always succeeds or throws UsernameNotFoundException. That assumption seems to be changing with JENKINS-55813, where User#impersonate can now also throw DisabledException. This change in behavior implies a need to revise the previous implementation of SECURITY-372 to take into account this new behavior by also catching DisabledException, but that work does not seem to have been done by members of the CERT team. The result is, from the perspective of the end user, a regression. Merely adding the user name to the error message does not seem to resolve the underlying problem.

[jglick]

assumes that User#impersonate always succeeds or throws UsernameNotFoundException.

That was a bad assumption even before #60. The caller should have been handling AuthenticationException (or simply RuntimeException) and behaving gracefully.

[jvz]

I did a survey of all the uses of User#impersonate back when I was working on JENKINS-55813, and every single usage I could find did not appear to make sense to allow to work when User#impersonate fails. If the user account has been locked or disabled, that was intentional, and there's a system property documented to disable that check if you have a broken LDAP setup.

And given the lack of detailed reproduction steps (particularly related to LDAP config), I don't see how any additional tests would be useful here.

[l3ender]

Hello, and thanks for the work on this issue. I was the one who created the JIRA issue and am wondering if I can provide any additional detail to help identify the problem.

The problem does seem to be build specific and I believe this is because the issue is related to a specific user's LDAP state, and that user had run some builds but not others. The issue (for us) manifests when the email plugin runs, which (I believe) tries to email all users since last successful build. But I'm not sure how to see what users are involved so I cannot debug any more on what their LDAP state is.

If anyone has any ideas on how I can add meaningful/helpful detail, please advise. Thanks!

[jvz]

This patch will fix the exception message so you'll at least see which username causes the problem. Then we can figure out an appropriate fix that will probably be needed for the mailer plugin since I can imagine a scenario already where this would get annoying (which might be similar to the one you're seeing).

[basil]

That was a bad assumption even before #60.

I don't disagree, Jesse. The problem is that this bad assumption didn't cause any problems for users before JENKINS-55813, but it does now, and the CERT team hasn't made any effort to deal with this, nor have they given plugin maintainers guidance as to how to adapt to this change in behavior. This puts plugin maintainers in an awkward situation with regard to dealing with user complaints. Ideally the CERT team would revise the implementation of SECURITY-372 to avoid this bad assumption and fix the user-visible regression. The next best thing would be for the CERT team to provide guidance as to how to adapt the implementation of SECURITY-372 with regard to the changes in JENKINS-55813, which would at least solve the problem for end users (but would still be an annoyance for plugin maintainers).

[jvz]

In fact, that might be a separate bug in the mailer plugin in how it chooses who to email about a build. For example, whenever I release a plugin, I sometimes get emails from random Jenkins instances with my plugin's commits included in their changelog. The fact that the mailer plugin (or whichever plugin) is so extensive in who it emails may itself be a bug.

[jglick]

The CERT team may have offered fixes years ago for security advisories in mailer & email-ext (I may have been an original author of some of that; do not recall the exact history), but that does not make them maintainers in perpetuity of those plugins.

The fix should be simple enough: catch RuntimeException or AuthenticationException instead of UsernameNotFoundException, or add another catch block for other errors that are not of the type “no such user”—could be a failure to contact the LDAP server, etc.

[basil]

The CERT team may have offered fixes years ago for security advisories in mailer & email-ext […], but that does not make them maintainers in perpetuity of those plugins.

Of course it does not. Yes JENKINS-64629 is arguably a preëxisting issue in the SECURITY-372 logic that was merely exposed by JENKINS-55813. But on the other hand, it was JENKINS-55813 that exposed it: the SECURITY-372 logic in Mailer and Email Extension might have been brittle with respect to such changes, but it had at least worked prior to JENKINS-55813. My conclusion is that the problem isn't JENKINS-55813 per se, but rather the CERT team's decided lack of caution when modifying such a fragile subsystem, namely the due diligence of searching for and updating existing callers.

The fix should be simple enough

Agreed. This needs to be done for both Mailer and Email Extension.

[jglick]

@jvz care to try ^^^?

[jvz]

Sure, I'll look at what plugins are calling this improperly.