[JENKINS-39402] Cap the number of group headers printed by AccessDeniedException2 #2727

jglick · 2017-01-27T21:19:29Z

@reviewbybees

…ssDeniedException2.

jglick · 2017-01-27T21:19:57Z

test/src/test/java/hudson/security/AccessDeniedException2Test.java

+            r.createWebClient().login("user").goTo("confgure");
+            fail("should not have been allowed to access anything");
+        } catch (FailingHttpStatusCodeException x) {
+            assertEquals(HttpURLConnection.HTTP_FORBIDDEN, x.getStatusCode());


Without fix, fails here with a 500 from Jetty as reported in the field.

jglick · 2017-01-27T21:20:13Z

test/src/test/java/hudson/security/AccessDeniedException2Test.java

+            }
+            assertThat("capped at a reasonable number", reportedGroups, Matchers.<List<String>>allOf(
+                Matchers.<String>hasSize(11), // 10 groups plus final warning
+                Matchers.<String>hasItem("<991 more>"))); // 1000 + SecurityRealm.AUTHENTICATED_AUTHORITY.getAuthority() - 10


order may be arbitrary

daniel-beck

Looks good, although I'm not convinced that with the low limit any useful behavior in nontrivial security realms (where this is actually needed) will remain.

daniel-beck · 2017-01-28T22:12:30Z

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

@@ -12,6 +12,9 @@
 * @author Kohsuke Kawaguchi
 */
 public class AccessDeniedException2 extends AccessDeniedException {
+
+    private static final int MAX_REPORTED_AUTHORITIES = 10;


Seems to be too low to be useful, especially given crazy behaviors such as https://github.com/jenkinsci/ldap-plugin/blob/4ced65b080ff3eb81ad23916ccb6133077b80185/src/main/java/hudson/security/LDAPSecurityRealm.java#L1039

tbh I'm not sure reporting this is ever useful, given the whoAmI url

rsandell · 2017-01-30T11:15:26Z

🐝

jglick · 2017-01-30T14:31:32Z

any useful behavior in nontrivial security realms

I think this is more a place to start diagnosis. You can always go to /whoAmI etc. to see the gory details. I do not want to print many headers for fear of pushing against limits in haproxy and the like.

daniel-beck · 2017-01-30T15:01:50Z

Should this not rather count both headers and individual authority name length as well? I.e. cap individual names at ~50 chars long or something?

oleg-nenashev

I agree with @daniel-beck that the limit seems to be too small. In order to avoid negative, the limit should be modifiable via SystemProperties at least

jglick · 2017-01-30T16:47:05Z

These suggestions are more effort than they are worth. I would rather just delete the functionality altogether.

jtnord · 2017-01-30T22:43:39Z

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

@@ -38,8 +41,14 @@ public AccessDeniedException2(Throwable t, Authentication authentication, Permis
     */
    public void reportAsHeaders(HttpServletResponse rsp) {
        rsp.addHeader("X-You-Are-Authenticated-As",authentication.getName());
-        for (GrantedAuthority auth : authentication.getAuthorities()) {
-            rsp.addHeader("X-You-Are-In-Group",auth.getAuthority());
+        GrantedAuthority[] authorities = authentication.getAuthorities();


do we even need to report this given there is a whoAmI which reports this in a much nicer way (and most users won't check the headers, and most scripts can't take any action)

jtnord · 2017-01-30T22:45:51Z

These suggestions are more effort than they are worth. I would rather just delete the functionality altogether.

I do not see a use for this funcationality given there is a whoAmI so +1 to that. but 🐝 as it stands as it fixes the issue

jtnord

Fixes the issue - but not sure it makes sense to leave something capped in the code that you can get from whoAmI with a full list.

…-39402

… omitting them altogether by default. Test still reproduces the original issue when flag is set on: … org.eclipse.jetty.server.HttpChannel$CommitCallback failed WARNING: Commit failed java.io.IOException: Response header too large at org.eclipse.jetty.http.HttpGenerator.generateResponse(HttpGenerator.java:402) at org.eclipse.jetty.server.HttpConnection$SendCallback.process(HttpConnection.java:655) at … at hudson.security.AccessDeniedHandlerImpl.handle(AccessDeniedHandlerImpl.java:57) at … Caused by: java.nio.BufferOverflowException at java.nio.HeapByteBuffer.put(HeapByteBuffer.java:189) at java.nio.ByteBuffer.put(ByteBuffer.java:859) at org.eclipse.jetty.http.HttpGenerator.putTo(HttpGenerator.java:1087) at org.eclipse.jetty.http.HttpGenerator.generateHeaders(HttpGenerator.java:705) at org.eclipse.jetty.http.HttpGenerator.generateResponse(HttpGenerator.java:387) ... 66 more java.lang.AssertionError: expected:<403> but was:<500> at org.junit.Assert.fail(Assert.java:88) at org.junit.Assert.failNotEquals(Assert.java:834) at org.junit.Assert.assertEquals(Assert.java:645) at org.junit.Assert.assertEquals(Assert.java:631) at hudson.security.AccessDeniedException2Test.youAreInGroupHeaders(AccessDeniedException2Test.java:56)

oleg-nenashev · 2017-01-31T18:37:26Z

I would rather like to see a test demonstrating the correct behavior in both modes than just removal of the test logic for the disabled logic

jglick · 2017-01-31T18:58:50Z

a test demonstrating the correct behavior in both modes

If it is disabled by default, who cares? There was no test before and patch merely conditionally runs the original code. The test logic in the initial version of the patch was to verify that the more complicated capping logic worked.

oleg-nenashev · 2017-02-02T19:31:05Z

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

@@ -38,8 +43,10 @@ public AccessDeniedException2(Throwable t, Authentication authentication, Permis
     */
    public void reportAsHeaders(HttpServletResponse rsp) {
        rsp.addHeader("X-You-Are-Authenticated-As",authentication.getName());
-        for (GrantedAuthority auth : authentication.getAuthorities()) {
-            rsp.addHeader("X-You-Are-In-Group",auth.getAuthority());
+        if (REPORT_GROUP_HEADERS) {


If false, I would rather return a message like "Disabled due to JENKINS-39402; use /whoAmI to diagnose groups. Or set the XXX system property to true" within the Header value

jtnord

🐝

…-39402

[FIXED JENKINS-39402] Cap the number of group headers printed by Acce…

d6f7e41

…ssDeniedException2.

jglick commented Jan 27, 2017

View reviewed changes

daniel-beck approved these changes Jan 28, 2017

View reviewed changes

oleg-nenashev requested changes Jan 30, 2017

View reviewed changes

jtnord reviewed Jan 30, 2017

View reviewed changes

jtnord approved these changes Jan 30, 2017

View reviewed changes

jglick added 2 commits January 31, 2017 13:12

Merge branch 'master' into AccessDeniedException2-header-size-JENKINS…

0d99f1c

…-39402

oleg-nenashev reviewed Feb 2, 2017

View reviewed changes

jtnord approved these changes Feb 2, 2017

View reviewed changes

jglick added 2 commits February 3, 2017 12:09

Merge branch 'master' into AccessDeniedException2-header-size-JENKINS…

021912b

…-39402

@oleg-nenashev requests a warning about the disabled headers.

20ceadb

jglick added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Feb 6, 2017

daniel-beck merged commit f8b26a3 into jenkinsci:master Feb 10, 2017

jglick deleted the AccessDeniedException2-header-size-JENKINS-39402 branch February 10, 2017 16:09

daniel-beck added a commit that referenced this pull request Feb 13, 2017

Noting #2722 #2723 #2727 #2732 #2735 #2737 #2738 #2739 #2744

f6005c3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-39402] Cap the number of group headers printed by AccessDeniedException2 #2727

[JENKINS-39402] Cap the number of group headers printed by AccessDeniedException2 #2727

jglick commented Jan 27, 2017

jglick Jan 27, 2017

jglick Jan 27, 2017

daniel-beck left a comment

daniel-beck Jan 28, 2017

jtnord Jan 30, 2017

rsandell commented Jan 30, 2017

jglick commented Jan 30, 2017

daniel-beck commented Jan 30, 2017

oleg-nenashev left a comment

jglick commented Jan 30, 2017

jtnord Jan 30, 2017

jtnord commented Jan 30, 2017

jtnord left a comment

oleg-nenashev commented Jan 31, 2017

jglick commented Jan 31, 2017

oleg-nenashev Feb 2, 2017

jtnord left a comment

[JENKINS-39402] Cap the number of group headers printed by AccessDeniedException2 #2727

[JENKINS-39402] Cap the number of group headers printed by AccessDeniedException2 #2727

Conversation

jglick commented Jan 27, 2017

jglick Jan 27, 2017

Choose a reason for hiding this comment

jglick Jan 27, 2017

Choose a reason for hiding this comment

daniel-beck left a comment

Choose a reason for hiding this comment

daniel-beck Jan 28, 2017

Choose a reason for hiding this comment

jtnord Jan 30, 2017

Choose a reason for hiding this comment

rsandell commented Jan 30, 2017

jglick commented Jan 30, 2017

daniel-beck commented Jan 30, 2017

oleg-nenashev left a comment

Choose a reason for hiding this comment

jglick commented Jan 30, 2017

jtnord Jan 30, 2017

Choose a reason for hiding this comment

jtnord commented Jan 30, 2017

jtnord left a comment

Choose a reason for hiding this comment

oleg-nenashev commented Jan 31, 2017

jglick commented Jan 31, 2017

oleg-nenashev Feb 2, 2017

Choose a reason for hiding this comment

jtnord left a comment

Choose a reason for hiding this comment