Draft for adding insecure certificates option #68
Conversation
added the insecure option to the type Request
added the insecure option to the newRequest function, by default it is false (meaning secure)
is the option for insecure is set, then the insecure options for curl or set. That being to disable peer, and host verification.
added flags for setting insecure options, and an error for secure failure.
added the error to check for a resend request., because sometimes Negotiate authorization handshakes may return this error
added the WinHttpSetOption function from the dynamic library winhttp. This will allow us the set security options on the request.
Added the WINHTTP_OPTION_SECURITY_FLAGS for setting security options, and the ERROR_INTERNET_INVALID_CA error, for catching another security error
This implementation allows for insecure connection if specified. This required adding some more definitions, and procedures to windefs.nim
adding the setAllowsAnyHTTPSCerfiticate function.
|
This is missing tests. Is it possible to create a debug test server with a self signed cert? |
|
I've made a test server yes. I didn't add the tests, because I didn't know if you would want that or not. But yes. I can provide the tests. It might take a while though. Getting ready for work. |
This python server utilizes self signed certificates for HTTPS communication for testing the insecure ssl PR draft.
added self signed certificates utilizing openSSL.
|
I've added a test block "insecure" to tests.nim I've also added test server in python and I've also added self signed certificates utilizing openSSL. |
I've forgot to remove some variables for testing when making the test server.
|
So I've tried both of your recommendations. with setAllowsAnyHTTPSCertificate, however I still get the error attached as a screenshot.
|
|
Yeah setAllowsAnyHTTPSCertificate is a private API so apple can and probably just removed it or it might have different arguments now. We need to find an official way to do this that is documented by Apple. |
|
So, if my research is correct. Apple API doesn't allow self signed certificates (Not without the private api). |
|
@Techno-Fox If you're still interested in this PR and would like your version merged, consider #79 as some feedback (no loop on Windows, smaller Python server, more clear name since I'm sure @treeform will have opinions too and you're welcome to share your thoughts. Just trying to save a bunch of back and forth. Not trying to steal your PR commit if you would like that accomplishment. |
|
Yes actually. I would still be interested in merging. Thank you for just not stealing. Accomplishments are always welcome. |
|
It's been a while. Mind catching me up on what's happening with the PR? |
|
If I remember correctly. During testing it didn't work without the loop on Windows. Unless someone has found a way to do so. |
|
I thought I did make the variable name to allowAnyHttpsCertificate? |
|
And how small do you want the python server? |
It is still Line 18 in 1a009e3 |
A loop is not needed, just a second call to WinHttpSendRequest, see https://github.com/treeform/puppy/pull/79/files#diff-1ebf7a29255776895f8c7e956c2c1a4ffa73ceec647a3723a5c75e0712a253ff Note I want to ignore the retry error, unless we can reproduce and test it. It is not needed in my testing for self-signed certs. |
The server in this PR includes some big ###### comments that don't add anything to such a small test file. I'd like to just have the simple Python. That's probably the only thing I'd prefer changed. |
No problem, we'll get this merged soon here with just a few tweaks. |
|
K. Thanks for letting me know what's needed. I'm just getting off work, so I'll see about fixing these possibly tomorrow (my time at least) |
Sounds good! |
messed up on commit pull, this might fix confilcts
Been a while working on this repository, hopefully this fixes an conflicts.
|
sorry that took so many tries, made a python script that could look at the diffs of each commit, and well it was an old script, so I had to fix some bugs. |
|
Let me know if I've forgot anything |
|
alright, I'll get on it |
Alright somethings I'll have to fix in my script, but I went back and edited them manually. Please let me know if I've forgotten anything.
| @@ -79,22 +86,22 @@ proc WinHttpConnect*( | |||
| ): HINTERNET {.dynlib: "winhttp".} | |||
|
|
|||
| proc WinHttpOpenRequest*( | |||
There was a problem hiding this comment.
Things look close to passing tests, close to done here which is gerat. I think the remaining issue is that WinHttpOpenRequest needs the full correct function parameters back (they're deleted currently in the PR).
See https://github.com/treeform/puppy/runs/7845915480?check_suite_focus=true
I've added the function parameters requested, must've forgotten about them during manual editing.
|
I've added the function parameters |
| @@ -118,6 +118,44 @@ proc fetch*(req: Request): Response {.raises: [PuppyError].} = | |||
| PuppyError, "WinHttpSendRequest error: " & $GetLastError() | |||
There was a problem hiding this comment.
Since we're checking error and possibly retrying, we do not want to raise an exception yet. This should be removed to get windows passing the tests (https://github.com/treeform/puppy/runs/7869416921?check_suite_focus=true)
There was a problem hiding this comment.
It looks like this is probably fix needed to get things ready to merge.
There was a problem hiding this comment.
alright I'll remove it.
Removed the exception at the line specified to remove at.
|
sorry for the wait. Had work. |
|
Thank you for this PR. Its a big one. We appreciate you working with us to get it in. Congrats! |
|
Welcome. |
The linux and windows are working, however I'm having issues with the macos side of things. I don't have much experience with macos, so please forgive my ignorance. I look forward to discussing this with you. Also. Sorry if my replies seem a little late. Work can be long.