Skip to content
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.

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

Add HTTPS support #2

Closed
rweigel opened this issue Feb 27, 2019 · 15 comments
Closed

Add HTTPS support #2

rweigel opened this issue Feb 27, 2019 · 15 comments
Assignees

Comments

@rweigel
Copy link
Collaborator

rweigel commented Feb 27, 2019

Add support for serving over HTTPS.

Branch is ...

@rweigel
Copy link
Collaborator Author

rweigel commented Aug 28, 2020

Make sure web pages work.

@rweigel
Copy link
Collaborator Author

rweigel commented Aug 30, 2020

server.js should have two new command line arguments that are empty strings by default.

--cert
--key

If these are non-empty strings, they are assumed to be files and read. If read fails, exit. Otherwise, start https server.

In the documentation, we should add instructions for using https. For example, following https://nodejs.org/en/knowledge/HTTP/servers/how-to-create-a-HTTPS-server/

openssl genrsa -out key.pem
openssl req -new -key key.pem -out csr.pem
openssl x509 -req -days 9999 -in csr.pem -signkey key.pem -out cert.pem
rm csr.pem
./hapi-server --cert /path/to/cert.pem --key /path/to/key.pem 

A test should be added that creates the certificates and runs the test suite using an https server.

@KSR4599
Copy link
Contributor

KSR4599 commented Sep 13, 2020

The behavior of execSync does seem to be uniform across all the platforms. The execSync seems to work flawlessly in the UNIX environment. But, in windows, when running the subsetting tests, the execSync is failing to execute the command. Whereas using exec/spawn asynchronously works well. Can we run the respective command based on the OS (process.env)?

@rweigel
Copy link
Collaborator Author

rweigel commented Sep 13, 2020 via email

@KSR4599
Copy link
Contributor

KSR4599 commented Sep 13, 2020

Your first sentence contradicts your second sentence. Anyway, I think that in all cases we should use spawn. In the tests, await should be used to block. This code may have some hints if you run into issues: https://www.npmjs.com/package/cross-spawn

I am sorry. I meant to say the behavior is not constant. Will try cross-spawn and give an update.

@KSR4599
Copy link
Contributor

KSR4599 commented Sep 14, 2020

The initial problem with spawn in ubuntu was that the function is not halting for the subprocess to fetch the data and terminate. I have fixed this by making sure the function returns only after the data is returned and the child is exited Yet, the same approach in windows is throwing the ENONET error. I tried the following workarounds :

  1. Command having spaces in it

  2. Usage of option {shell:true}

  3. Tried cross-spawn and spawn-cmd

Still, the error persists in windows and trying to resolve it.

@rweigel
Copy link
Collaborator Author

rweigel commented Sep 14, 2020 via email

@KSR4599
Copy link
Contributor

KSR4599 commented Sep 14, 2020

Would you post the minimal code that shows error in Windows? For example, something that spawns a call to python that executes a script that only prints a string.

On Mon, Sep 14, 2020 at 1:05 PM K SRIKAR REDDY @.***> wrote: The initial problem with spawn in ubuntu was that the function is not halting for the subprocess to fetch the data and terminate. I have fixed this by making sure the function returns only after the data is returned and the child is exited https://nodejs.org/api/child_process.html#child_process_child_process Yet, the same approach in windows is throwing the ENONET error. I tried the following workarounds : 1. Command having spaces <nodejs/node-v0.x-archive#25895> in it 2. Usage of option {shell:true} <nodejs/node-v0.x-archive#2318 (comment)> 3. Tried cross-spawn https://www.npmjs.com/package/cross-spawn and spawn-cmd https://www.npmjs.com/package/spawn-cmd Still, the error persists in windows and trying to resolve it. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAUQ57V6GNXAPWZMPEKRHRTSFZEOJANCNFSM4G2SKZVQ .

Actually just fixed the error. This is a path issue related to $NODEEXE having spaces within it. In windows, $NODEEXE breaks down to "C:\Program Files\nodejs\node.exe" in the background where there is a space in between the command. This error is not encountered in ubuntu since $NODEEXE breaks down to something like "/usr/bin/node" (no spaces). Enclosing the $NODEEXE within the quotes i.e
\"$NODEEXE\" fixed the issue.

@KSR4599
Copy link
Contributor

KSR4599 commented Sep 21, 2020

Actually, it turns out that, the command to generate the catalog file, is not being treated as command, but as file in the getmetadata function. There seems to be issue with commandExistsSync to treat cd as a command in windows. Working on the way to modify the command inorder to make it work.

@rweigel
Copy link
Collaborator Author

rweigel commented Sep 21, 2020

OK. But this does make me wonder about the wisdom of having to check if it is a file or command. Probably the schema should be changed so that one gives either metadata (JSON), metadataFile (string; local path), or metadataURL (string; URL) instead of allowing metadata to be any of the three.

@KSR4599
Copy link
Contributor

KSR4599 commented Sep 21, 2020

Yes, that would also remove the need to check for all the three conditions each time. Going with the option of metadataFile ( i.e local path) would be straight forward. For which we need to generate the catalog file well in advance before giving it as the local path to the catalog section.

@rweigel
Copy link
Collaborator Author

rweigel commented Sep 21, 2020

I suggest doing this then. You'll need to update all of the .json files in the metadata/ directory.

Will the exec function work in Windows if we make this change? I don't like the fact that the buffer size is hard-wired. It seems spawn is more appropriate for an unknown response size.

@KSR4599
Copy link
Contributor

KSR4599 commented Sep 21, 2020

Yes, I will work on updating the files in metadata dir. As you mentioned, usage of spawn shall remove the need to hard-code the buffer limit. I will appropriately replace exec with spawn.

@KSR4599
Copy link
Contributor

KSR4599 commented Sep 23, 2020

Update: I have implemented the suggested approach of changing the schema. Changed the schema such that the one gives either metadata File (local path) or metadata URL. I did not omit the option of a metadata URL as it seems inefficient to do so. The same code to process the metadata URL must be placed elsewhere with an added disadvantage that any future metadata URL must be manually added to the list of URLs to be processed inorder to get it stored in a separate JSON file. It seems valid to get this process done on the fly based on the type of the catalog (either localpath or URL) provided. Having said that, now all the tests are passing on all the three platforms locally. Yet, facing few issues with windows in Travis-ci. In the process of debugging :

  1. Tested minimal node.js application with version 6.1.6. It works.
  2. Tested the Hapi server with the latest version of node.js. It works.

The error associated with Hapi server for node v6.1.6 :

gyp ERR! build error

gyp ERR! stack Error: C:\Windows\Microsoft.NET\Framework\v4.0.30319\msbuild.exe failed with exit code: 1

gyp ERR! stack at ChildProcess.onExit (C:\ProgramData\nvs\node\6.16.0\x64\node_modules\npm\node_modules\node-gyp\lib\build.js:276:23)

gyp ERR! stack at emitTwo (events.js:106:13)

gyp ERR! stack at ChildProcess.emit (events.js:191:7)

gyp ERR! stack at Process.ChildProcess._handle.onexit (internal/child_process.js:219:12)

gyp ERR! System Windows_NT 10.0.17763

gyp ERR! command "C:\ProgramData\nvs\node\6.16.0\x64\node.exe" "C:\ProgramData\nvs\node\6.16.0\x64\node_modules\npm\node_modules\node-gyp\bin\node-gyp.js" "rebuild" "--release"

gyp ERR! cwd C:\Users\travis\build\drunkenlord\hapi_test\node_modules\fibers

gyp ERR! node -v v6.16.0

gyp ERR! node-gyp -v v3.4.0

gyp ERR! not ok

Seems like a node-gyp error associated with node version 6.1.6 on travis. Trying to resolve it.

@KSR4599
Copy link
Contributor

KSR4599 commented Sep 26, 2020

@KSR4599 KSR4599 closed this as completed Sep 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants