Skip to content

[JENKINS-55557] Add support for state parameter #107

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Apr 12, 2019
Merged

[JENKINS-55557] Add support for state parameter #107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8d51832
[JENKINS-55557] Add support for state parameter
jkakavas Jan 13, 2019
df16ae3
Remove the dependency to java.util.Base64
jkakavas Jan 14, 2019
962a063
Fix sessionFixation test
jkakavas Jan 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 39 additions & 2 deletions src/main/java/org/jenkinsci/plugins/GithubSecurityRealm.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ of this software and associated documentation files (the "Software"), to deal
import java.io.IOException;
import java.net.InetSocketAddress;
import java.net.Proxy;
import java.security.SecureRandom;
import java.util.Arrays;
import java.util.HashSet;
import java.util.Set;
Expand Down Expand Up @@ -337,6 +338,9 @@ public String getOauthScopes() {

public HttpResponse doCommenceLogin(StaplerRequest request, @QueryParameter String from, @Header("Referer") final String referer)
throws IOException {
// https://tools.ietf.org/html/rfc6749#section-10.10 dictates that probability that an attacker guesses the string
// SHOULD be less than or equal to 2^(-160) and our Strings consist of 65 chars. (65^27 ~= 2^160)
final String state = getSecureRandomString(27);
String redirectOnFinish;
if (from != null && Util.isSafeToRedirectTo(from)) {
redirectOnFinish = from;
Expand All @@ -347,18 +351,19 @@ public HttpResponse doCommenceLogin(StaplerRequest request, @QueryParameter Stri
}

request.getSession().setAttribute(REFERER_ATTRIBUTE, redirectOnFinish);
request.getSession().setAttribute(STATE_ATTRIBUTE, state);

Set<String> scopes = new HashSet<>();
for (GitHubOAuthScope s : getJenkins().getExtensionList(GitHubOAuthScope.class)) {
scopes.addAll(s.getScopesToRequest());
}
String suffix="";
if (!scopes.isEmpty()) {
suffix = "&scope="+Util.join(scopes,",");
suffix = "&scope="+Util.join(scopes,",")+"&state="+state;
} else {
// We need repo scope in order to access private repos
// See https://developer.github.com/v3/oauth/#scopes
suffix = "&scope=" + oauthScopes;
suffix = "&scope=" + oauthScopes +"&state="+state;
}

return new HttpRedirect(githubWebUri + "/login/oauth/authorize?client_id="
Expand All @@ -372,13 +377,27 @@ public HttpResponse doCommenceLogin(StaplerRequest request, @QueryParameter Stri
public HttpResponse doFinishLogin(StaplerRequest request)
throws IOException {
String code = request.getParameter("code");
String state = request.getParameter(STATE_ATTRIBUTE);
String referer = (String)request.getSession().getAttribute(REFERER_ATTRIBUTE);
String expectedState = (String)request.getSession().getAttribute(STATE_ATTRIBUTE);

if (code == null || code.trim().length() == 0) {
Log.info("doFinishLogin: missing code.");
return HttpResponses.redirectToContextRoot();
}

if (state == null){
Log.info("doFinishLogin: missing state parameter from Github response.");
return HttpResponses.redirectToContextRoot();
} else if (expectedState == null){
Log.info("doFinishLogin: missing state parameter from user's session.");
return HttpResponses.redirectToContextRoot();
} else if (!state.equals(expectedState)){
Log.info("state parameter value ["+state+"] does not match the expected one ["+expectedState+"]");
return HttpResponses.redirectToContextRoot();
}


String accessToken = getAccessToken(code);

if (accessToken != null && accessToken.trim().length() > 0) {
Expand Down Expand Up @@ -460,6 +479,21 @@ private String getAccessToken(@Nonnull String code) throws IOException {
return null;
}

/**
* Generates a random URL Safe String of n characters
*/
private String getSecureRandomString(int n) {
if (n < 0){
throw new IllegalArgumentException("Length must be a positive integer");
}
// See RFC3986
final String urlSafeChars = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789.-_";
final StringBuilder sb = new StringBuilder();
for (int i = 0; i < n ; i++){
sb.append(urlSafeChars.charAt(SECURE_RANDOM.nextInt(urlSafeChars.length())));
}
return sb.toString();
}
/**
* Returns the proxy to be used when connecting to the given URI.
*/
Expand Down Expand Up @@ -789,6 +823,9 @@ static Jenkins getJenkins() {
private static final Logger LOGGER = Logger.getLogger(GithubSecurityRealm.class.getName());

private static final String REFERER_ATTRIBUTE = GithubSecurityRealm.class.getName()+".referer";
private static final String STATE_ATTRIBUTE = "state";

private static final SecureRandom SECURE_RANDOM = new SecureRandom();

/**
* Asks for the password.
Expand Down
3 changes: 2 additions & 1 deletion src/test/java/org/jenkinsci/plugins/GithubAccessTokenPropertySEC797Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,8 @@ private void onUserTeams(HttpServletRequest req, HttpServletResponse resp) throw

private void onLoginOAuthAuthorize(HttpServletRequest req, HttpServletResponse resp) throws IOException {
String code = "test";
resp.sendRedirect(jenkinsRule.getURL() + "securityRealm/finishLogin?code=" + code);
String state = req.getParameter("state");
resp.sendRedirect(jenkinsRule.getURL() + "securityRealm/finishLogin?code=" + code + "&state=" + state);
}

private void onLoginOAuthAccessToken(HttpServletRequest req, HttpServletResponse resp) throws IOException {
Expand Down
3 changes: 2 additions & 1 deletion src/test/java/org/jenkinsci/plugins/GithubAccessTokenPropertyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,8 @@ private void onUserTeams(HttpServletRequest req, HttpServletResponse resp) throw

private void onLoginOAuthAuthorize(HttpServletRequest req, HttpServletResponse resp) throws IOException {
String code = "test";
resp.sendRedirect(jenkinsRule.getURL() + "securityRealm/finishLogin?code=" + code);
String state = req.getParameter("state");
resp.sendRedirect(jenkinsRule.getURL() + "securityRealm/finishLogin?code=" + code + "&state=" + state);
}

private void onLoginOAuthAccessToken(HttpServletRequest req, HttpServletResponse resp) throws IOException {
Expand Down