Closed nanoscopic closed 5 years ago
bergmannf
commented
5 years ago Just a FYI as I got curious about not being able to read crt and pem files in Javascript - I tested it in jsfiddle and that worked without problems for me (Firefox 60 + Chromium 68).
The files were certificates in both cases and my browser are not the most recent version I think - so maybe that's why it worked for me?
So if we want to change to file upload in the future it might still be possible after all.
vitoravelino
commented
5 years ago Just a FYI as I got curious about not being able to read crt and pem files in Javascript - I tested it in jsfiddle and that worked without problems for me (Firefox 60 + Chromium 68).
@bergmannf I was also able to reproduce it as you've mentioned, also in this codepen example and inside velum with a quick copy and paste. I've used Firefox 60 (leap15), 61 (win10) and Chrome 68 (leap15, win10).
The file upload could not be used for the ldap external auth as modern browsers block getting the contents of a file selected for upload in javascript when that file has the extension ".crt" or ".pem".
@nanoscopic could you help us to reproduce it or point out an example of that happening?
nanoscopic
commented
5 years ago I looked at the code you have linked Vitor; looking at it there is one difference between it and what I did; the difference is that it is tied to "onchange" versus I was trying to read it when a file has already been selected. I considered doing that but did not think it would make a difference.
It still concerns me that crt and pem are blocked but txt is not when reading it later, not onchange. Why block it in one scenario but not another? I think it is painting us into a corner to assume it will continue working with onchange in the future.
vitoravelino
commented
5 years ago ... I was trying to read it when a file has already been selected.
By that you mean when the user clicks on the "test connection" button? Something like this? For me it worked the same way in both cases.
nanoscopic
commented
5 years ago Using the demos I see the same thing being said, that pem files can be read through JS using the same mechanism I tried.
Despite this, when I tested in the system itself, within a modified velum, it definitely did not work unless the file did not end in '.pem'. I searched the internet to attempt to find mention of this "security feature" in Firefox but I see no evidence of such behavior. I grep'd through the source code of Firefox ( gecko ) to see if I could locate the code that causes the failure as well; I as not able to find code that would cause it.
I could change the system back to test this again and further look into that particular aspect, but as explained in more detail via email; it is undesirable to use a file upload for external ldap authentication certs. It will soon be changed to be a button to get the cert from the server directly, and that will be more straightforward to do using a textarea or hidden field instead of a file upload.
As a result, I see no reason to spend more effort getting reading of the file browse via JS working within the external ldap auth feature.
The change from a text box to enter certificate content to a file upload broke the functionality of the LDAP external auth feature. That feature requires the inforation entered to be tested to connect to the ldap server before saving. No one tested that after the file upload change and hence it is broken and had to be updated. These changes make it work once more.
The file upload could not be used for the ldap external auth as modern browsers block getting the contents of a file selected for upload in javascript when that file has the extension ".crt" or ".pem". As a result the only way to fetch the cert to use it for the check would be to upload it to the backend somewhere temporarily.
The cert for external auth, though, should never have been changed to a file upload. The planned addition is a button to fetch from the server automatically and confirm to the user the fingerprint and information of the cert. Connecting the external auth to the other certificate handling in the system is not appropriate at this point in time.
This change also includes another fix for ldap connection test to be able to properly fetch the password. That is another breaking change that was not detected, also because there is no automated UI capybara test against the ldap connection stuff. An automated test to prevent this from breaking repeatedly will need to be added in the future as well. It is not included in this commit intentionally. This commit is intended just to get the feature back into a working state having been left broken.