Skip to content

Conversation

joelanders
Copy link
Contributor

@joelanders joelanders commented Apr 24, 2016

Ok, I think this is all working.
Be careful to hit the endpoint with an explicit https, eg: https://localhost:whatever as I wasted a bit of time on this :p

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 61.779% when pulling 894075c on feature/https_endpoint into 0c89c56 on master.

@joelanders joelanders force-pushed the feature/https_endpoint branch from 894075c to e268de2 Compare May 4, 2016 20:43
@joelanders joelanders force-pushed the feature/https_endpoint branch from e268de2 to 8ef4dc3 Compare May 4, 2016 20:46
@joelanders joelanders changed the title [NOT READY TO MERGE] Add TLS endpoint Add TLS endpoint May 4, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 61.162% when pulling e268de2 on feature/https_endpoint into 6f1eb71 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 61.223% when pulling 8ef4dc3 on feature/https_endpoint into 6f1eb71 on master.


collector_endpoints:
- {type: tls, port: 11443, cert: "private/ssl-key-and-cert.pem"}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be ideal if we could keep backward compatibility with old configuration files.

I am thinking that instead of having the endpoints be a list we could expand them to something like:

bouncer_tls_port: XXX
bouncer_tls_cert: XXX
bouncer_hsdir: XXX

collector_tls_port: XXX
collector_tls_cert: XXX
etc.

Copy link
Contributor Author

@joelanders joelanders May 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kinda thought it was nicer to group all the needed things into an object. It'd be even nicer if I made that object the Twister ServerFromString or whatever.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's keep this as is for the moment. I noticed you also added backward compatible changes so I think it's ok for the moment.

@hellais
Copy link
Member

hellais commented May 5, 2016

Overall this looks good 👍 I made a bit of comments for things that should be changed before we can merge this. Good stuff.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 61.261% when pulling 5fa24c3 on feature/https_endpoint into 6f1eb71 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 61.237% when pulling 2fa2848 on feature/https_endpoint into 6f1eb71 on master.

-keyout private/ssl-key.pem -out private/ssl-cert.pem \
-days 400 -nodes -subj '/CN=selfie'

cat private/ssl-key.pem private/ssh-cert.pem > private/ssl-key-and-cert.pem
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going to move this into the scripts directory when merging.

@hellais
Copy link
Member

hellais commented May 11, 2016

Other things to take into consideration:

  • If config.main.tor_hidden_service is set to false, then calls to getHSEndpoint will fail due to torconfig not being defined
  • Make sure the documentation is updated to reflect the changes in the configuration file
  • Update ooni-sysadmin to reflect the changes in this repo

@joelanders joelanders changed the title Add TLS endpoint [WIP] Add TLS endpoint May 13, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 61.125% when pulling 35ccea7 on feature/https_endpoint into 6f1eb71 on master.

…i-backend into feature/https_endpoint

* 'feature/https_endpoint' of github.com:TheTorProject/ooni-backend:
  only assume HS endpoint when tor_hidden_service: true
  explain + bail out when inconsistent config
@hellais
Copy link
Member

hellais commented May 13, 2016

I created two tickets for the documentation/sysadmin tasks here:

I am going to to a final review of this branch and merge it.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 61.125% when pulling d7f9a4c on feature/https_endpoint into 6f1eb71 on master.

@hellais
Copy link
Member

hellais commented May 13, 2016

Now there is an issue with the fact that if you set tor_hidden_service: false and don't set the bouncer_endpoints or collector_endpoints to be a list (because for example you want to just run test helpers and not have a bouncer or a collector) it will try to iterate over a None-type:

    for endpoint_config in config.main.bouncer_endpoints:
TypeError: 'NoneType' object is not iterable

@hellais hellais merged commit 90ca955 into master May 13, 2016
@hellais
Copy link
Member

hellais commented May 13, 2016

I fixed the outstanding bugs and merged into master.

Thanks!

@hellais hellais deleted the feature/https_endpoint branch May 13, 2016 11:21
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