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

[WIP] northBound server in HTTPS and Verify ContextBroker Certificate #601

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

m4n3dw0lf
Copy link

Described here: https://github.com/m4n3dw0lf/SecureFiware
Not shure if need some adjustments or something to merge in the master branch, any feedback will be appreciated.

@fgalan
Copy link
Member

fgalan commented Apr 3, 2018

Instead of defining a protocol filed, I'd suggest to use the approach already implemented in PR #584 based in URL/HOST/POST variables, eg:

    if (process.env.IOTA_CB_URL) {
        config.contextBroker.url = process.env.IOTA_CB_URL;
    } else if (process.env.IOTA_CB_HOST) {
        config.contextBroker.url = "http://" + process.env.IOTA_CB_HOST;
        if (process.env.IOTA_CB_PORT) {
            config.contextBroker.url += ":" + process.env.IOTA_CB_PORT;
        }

What do you think @chicco785 ?

@chicco785
Copy link
Contributor

i agree that for the https support in url @m4n3dw0lf can leverage on #584

still, he wants to be able to specify certificates to be used,
and for that i didn't add support

@chicco785
Copy link
Contributor

basically i recommend him to rebase code using my PR and add the TLS verification support on top.

@fgalan
Copy link
Member

fgalan commented Apr 3, 2018

@chicco785 your PR (#584) was already merge into master, so I understand that rebase will be done with master branch, isn't it?

@chicco785
Copy link
Contributor

chicco785 commented Apr 3, 2018 via email

@m4n3dw0lf
Copy link
Author

m4n3dw0lf commented Apr 3, 2018

@fgalan and @chicco785

I will checkout with #584 as soon as possible and then apply the verify certificate parameter, also will fix those checks that failed in the CI, about the HTTPS support for the northBound.js express server, any comments?

@chicco785
Copy link
Contributor

@m4n3dw0lf i am not a TLS expert, my only comments are:

  • what happens when the context broker specified for a group of devices is not the same one in the main configuration?
  • it would be good to add mocha tests for this feature

@m4n3dw0lf m4n3dw0lf changed the title northBound server in HTTPS and Verify ContextBroker Certificate [WIP] northBound server in HTTPS and Verify ContextBroker Certificate Apr 4, 2018
@chicco785
Copy link
Contributor

@m4n3dw0lf not sure the scope (and duration) of your project. it may be interesting to use a certificate repository to retrieve them, so to decouple the management of certificates from their usage. for another project, i am looking into https://www.vaultproject.io

@fgalan
Copy link
Member

fgalan commented Oct 7, 2020

We are considering merging PR #831 and this PR may be impacted (see more details here). However, any potential conflict is very easy to solve (I understand @jason-fox may provide details, if needed).

Is this PR still active? @m4n3dw0lf @chicco785 do you agree in merging PR #831 before your PR?

@chicco785
Copy link
Contributor

@fgalan i was just commenting on it :) i have no clue about what's the state of this pr

@fgalan
Copy link
Member

fgalan commented Oct 7, 2020

@fgalan i was just commenting on it :) i have no clue about what's the state of this pr

Sorry, I thought you were involved somehow ;)

Thus, @m4n3dw0lf could you provide some feedback, please?

@fgalan fgalan mentioned this pull request Nov 10, 2020
@fgalan
Copy link
Member

fgalan commented Nov 19, 2020

We finally merged PR #831 in order not to block in absence of answer from @m4n3dw0lf

The PR would need some adaptation to solve the conflicts. Basically:

  1. Solve the conflict accepting your changes
  2. Change style with npm run prettier

(Please @jason-fox amend mi if I'm wrong)

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

Successfully merging this pull request may close these issues.

3 participants