Webui login

[kalaspuffar]

Hi @bertfrees

I've been working on helping @josteinaj to set up login flow for their pipeline installation. We ran into a wall when you could not set up an API connection with authid and secret key and I guess it is because it's not using an URL encoded Base64 string for signing.

This PR is trying to resolve this issue. I've separated it into two different commits. The first one was all changes that were required for me in order to build the Pipeline Webservice and WebUI. Some SNAPSHOT version changes and also I changed to the new URI for restlet required of some packages.

More over I added the feature to create a zip file of the WebUI package. Not required so I can remove that change if you like.

The next commit is all code changes. Mainly I've changed to use the Base64.getUrlEncoder().withoutPadding().encode() for the hash that should be compared, this makes the process more reliable. I also had to change the UI component verifying the login to use a known endpoint like /jobs using the default root it never ran any login flow and always returned 404 so the WebUI could never have worked with authid and secret key.

I hope that these changes are OK, I understand that changing the API could be bad if a lot of people have implemented it against the old one and if so we could gradually change or just implement the change with the endpoint to ensure that the WebUI works. Then again @josteinaj could not reliably get the API to work.

---EDIT
I forgot to mention that because the webservice was a bundle the make script never built it so I had to do maven install to a couple of packages in this order.

framework/bom
framework/parent
framework/utils/clientlib-java-jaxb
framework/webservice

But after that it started fine and I could run it with credentials and use the WebUI.
---EDIT

Best regards
Daniel

[bertfrees]

Nice, many thanks.

I notice that the framework was not a snapshot. The "bump versions" commit was missing. This is why you had to change that many versions in POMs. Next time let me know and I can fix it. Then you'll have less work.

Looking at the actual code changes in your second commit now... So if I understand correctly the main problem was a bug in the SystemStatus class of the web UI? Was the API change of the web service also really needed? Authentication works, the command line interface proves that, and it has also been successfully used using the API directly. The documentation explains that it is required to URL encode the hash string before including it as a URL query parameter. Could it be that the web UI does not do that step?

I don't want to change the API if it's not needed. What we can do at most is compute two versions of the hash, one with Base64.getEncoder() and one with Base64.getUrlEncoder(), and authenticate if the hash coming from the client is either of these two.

That by changing to java.util.Base64 we get rid of the commons-coded dependency is a good thing for sure.

[kalaspuffar]

Hi @bertfrees

I want to learn how to bump the version in the future, so starting the work on any components would be easier.

I've now implemented an extra URL parameter called urlsign, which is the URL encoded version of the hash.

So now we could handle both versions, and if you have a better suggestion of the name, I could change it. I've tested this with both the WebUI and the command-line tool, and it works with both. The CLI version I tested with had not been updated, so it should have been running with the old implementation.

Best regards
Daniel

[kalaspuffar]

Hi @bertfrees

I added some documentation also so the users could choose this alternative implementation option. But, of course, if we want to change going forward, we could replace the original documentation and gradually move over and deprecate the old implementation. But that is up to the project to decide.

Best regards
Daniel

[bertfrees]

I want to learn how to bump the version in the future, so starting the work on any components would be easier.

The version of pipeline-framework that was committed to the super project was daisy/pipeline-framework@25e35ee. To bump the version I would commit daisy/pipeline-framework@0176e05. To do this I would run the command:

source .git-utils/git-subrepo/.rc
git subrepo commit framework 0176e05

The solution with the two parameters is definitely better because we don't change the API. Also thanks for the documentation. That clarified some things for me.

But I'm still not sure why all this is needed. Why is the web UI unable to correctly pass the hash string (with the padding etc.)?

@rdeltour What do you think of this?

[kalaspuffar]

Hi @bertfrees

Well, there are multiple reasons why I think this is an improvement. First, if you tell people to do encoding that then could automatically be decoded in some systems then they need to double encode and so on which never is a good solution. Changing to a defined standard it's easier to implement and hopefully will introduce fewer issues.

https://datatracker.ietf.org/doc/html/rfc4648#section-5

I built a client a couple of years ago and this standard was chosen to solve a lot of issues that we ran into with different browsers and operating systems.

And when it comes to the client we need to change it anyway because the encoding was not complete.

for (int i = 0; i < hash.length(); i++) {
	// Base64 encoding uses + which we have to encode in URL parameters.
	// Hoping this for loop is more efficient than the equivalent replace("\\+","%2B") regex.
	c = hash.charAt(i);
	if (c == '+') hashEscaped += "%2B";
	else hashEscaped += c;
} 

This function only handles one of the issues. Slash and equal signs are interpreted differently by different parsers or query builders. So if we want to use the old format then all of them should be encoded.

Best regards
Daniel

[bertfrees]

https://datatracker.ietf.org/doc/html/rfc4648#section-5

Thanks for the pointer. Yes, I agree one encoding step less to do is less things that can go wrong and it would've been better if we had gone for that solution from the beginning.

This function only handles one of the issues. Slash and equal signs are interpreted differently by different parsers or query builders. So if we want to use the old format then all of them should be encoded.

So there's the bug! Thanks, this is what I wanted to know. I needed to know whether there is a problem in the client that can be fixed, or whether there is a fundamental problem with the web API. To be completely reassured, I'd like to first check if fixing this bug fixes the problem and is not too hard. In a second phase we can consider improving the API by introducing a new "urlsign" parameter. At first sight fixing the bug looks pretty straightforward. A few lines higher up in the code, URLEncoder.encode() is used to encode all the other query parameters.

if (parameters != null) {
    for (String name : parameters.keySet()) {
        try { url += URLEncoder.encode(name, "UTF-8") + "=" + URLEncoder.encode(parameters.get(name), "UTF-8") + "&"; }
        catch (UnsupportedEncodingException e) { throw new Pipeline2Exception("Unsupported encoding: UTF-8", e); }
    }
}

I wonder why the same technique is not used below to encode the "sign" parameter.

[bertfrees]

@@ -237,34 +237,44 @@ public class Pipeline2HttpClient {
 		
 		if (parameters != null) {
 			for (String name : parameters.keySet()) {
-				try { url += URLEncoder.encode(name, "UTF-8") + "=" + URLEncoder.encode(parameters.get(name), "UTF-8") + "&"; }
-				catch (UnsupportedEncodingException e) { throw new Pipeline2Exception("Unsupported encoding: UTF-8", e); }
+				try {
+					url += URLEncoder.encode(name, "UTF-8")
+						+ "=" + URLEncoder.encode(parameters.get(name), "UTF-8")
+						+ "&"; }
+				catch (UnsupportedEncodingException e) {
+					throw new Pipeline2Exception("Unsupported encoding: UTF-8", e); }
 			}
 		}
 		
 		if (hasAuth) {
-			String time = iso8601.format(new Date());
 
+			// add parameters "authid", "time" and "nonce"
+			parameters = new HashMap<>();
+			parameters.add("authid", username);
+			String time = iso8601.format(new Date());
+			parameters.add("time", time);
 			String nonce = "";
 			while (nonce.length() < 30)
 				nonce += (Math.random()+"").substring(2);
 			nonce = nonce.substring(0, 30);
+			parameters.add("nonce", nonce);
+			for (String name : parameters.keySet()) {
+				try {
+					url += name
+						+ "=" + URLEncoder.encode(parameters.get(name), "UTF-8")
+						+ "&"; }
+				catch (UnsupportedEncodingException e) {
+					throw new Pipeline2Exception("Unsupported encoding: UTF-8", e); }
+			}
 
-			url += "authid="+username + "&time="+time + "&nonce="+nonce;
-
-			String hash = "";
+			// add parameter "sign"
 			try {
-				hash = calculateRFC2104HMAC(url, secret);
-				String hashEscaped = "";
-				char c;
-				for (int i = 0; i < hash.length(); i++) {
-					// Base64 encoding uses + which we have to encode in URL parameters.
-					// Hoping this for loop is more efficient than the equivalent replace("\\+","%2B") regex.
-					c = hash.charAt(i);
-					if (c == '+') hashEscaped += "%2B";
-					else hashEscaped += c;
-				} 
-				url += "&sign="+hashEscaped;
+				String hash = calculateRFC2104HMAC(url, secret);
+				try {
+					url += "sign"
+						+ "=" + URLEncoder.encode(hash, "UTF-8"); }
+				catch (UnsupportedEncodingException e) {
+					throw new Pipeline2Exception("Unsupported encoding: UTF-8", e);
 
 			} catch (SignatureException e) {
 				throw new Pipeline2Exception("Could not sign request.");

[kalaspuffar]

Hi @bertfrees

Not sure if the solution is complete, but the changes seem reasonable.

Best regards
Daniel

[bertfrees]

@kalaspuffar webui v2.7.1 was released and should fix the issue (see also daisy/pipeline-webui#140). Next I will take your "urlsign" related changes and create a new PR from it.

[josteinaj]

It seems that orgaisy on docker hub does not build the webui automatically. Not a problem, I just assumed it were. In any case, I built it manually, tagged it and pushed it, so that it's now available as the docker image daisyorg/pipeline-webui:v2.7.1. I tested locally that it works.

[bertfrees]

For the "urlsign" related changes see this new PR: #637