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

Allow self-signed certs #4377

Merged
merged 3 commits into from Nov 5, 2017

Conversation

Projects
None yet
5 participants
@sbulen
Contributor

sbulen commented Oct 28, 2017

Realized while testing that the ssl check does not honor self-signed certs. It's possible some sites want to use self-signed. Also, not honoring self-signed will hinder folks' ability to test.

Tested in windows & linux, with cert & without.

Allow self-signed certs
Signed by Shawn Bulen, bulens@pacbell.net
@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Oct 29, 2017

Collaborator

I would not do that hardcoded because this are security issues and
since "Let's Encrypt" it's for the most cases (beside test cases) easy to get real certs.

Collaborator

albertlast commented Oct 29, 2017

I would not do that hardcoded because this are security issues and
since "Let's Encrypt" it's for the most cases (beside test cases) easy to get real certs.

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Oct 29, 2017

Contributor

To be clear, the choice whether to enable https is under admin control and is not hard-coded.

This is a helper function, used by the installer and the admin control panel, that determines whether the https options are disabled or not. This change enables an admin to use a self signed cert.

Contributor

sbulen commented Oct 29, 2017

To be clear, the choice whether to enable https is under admin control and is not hard-coded.

This is a helper function, used by the installer and the admin control panel, that determines whether the https options are disabled or not. This change enables an admin to use a self signed cert.

@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Oct 29, 2017

Collaborator

But the admin had not the controll if self signed is allowed or not.

Collaborator

albertlast commented Oct 29, 2017

But the admin had not the controll if self signed is allowed or not.

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Oct 29, 2017

Contributor

Corect. And now s/he has that choice.

Contributor

sbulen commented Oct 29, 2017

Corect. And now s/he has that choice.

@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Oct 29, 2017

Collaborator

Wher he can choose?
In my eyes you hard coded that self signed are allowed(he had not the choice if he want it or not).

Collaborator

albertlast commented Oct 29, 2017

Wher he can choose?
In my eyes you hard coded that self signed are allowed(he had not the choice if he want it or not).

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Oct 29, 2017

Contributor

The admin can choose whether to use https on the installer and in the acp.

The admin will know what type of cert s/he is using.

Adding settings for settings for settings is overkill and confusing. This change leaves the admin with greater flexibility without unnecessary complexity.

Contributor

sbulen commented Oct 29, 2017

The admin can choose whether to use https on the installer and in the acp.

The admin will know what type of cert s/he is using.

Adding settings for settings for settings is overkill and confusing. This change leaves the admin with greater flexibility without unnecessary complexity.

@tinoest

This comment has been minimized.

Show comment
Hide comment
@tinoest

tinoest Oct 29, 2017

Contributor

I don’t think this is a good change, self signed certs still throw the error when you visit the site. You can get dev certs from letsencrypt ( or other certificate authorities ) if you need them.

I can see this causing problems for those that don’t understand the difference between them.

Contributor

tinoest commented Oct 29, 2017

I don’t think this is a good change, self signed certs still throw the error when you visit the site. You can get dev certs from letsencrypt ( or other certificate authorities ) if you need them.

I can see this causing problems for those that don’t understand the difference between them.

@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Oct 29, 2017

Collaborator

In my eyes this "feature" needs to be able to disable (and this should be the default setting of this).

Collaborator

albertlast commented Oct 29, 2017

In my eyes this "feature" needs to be able to disable (and this should be the default setting of this).

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Oct 29, 2017

Contributor

I really think it should be up to the admin, and that we should not prohibit the admin from doing so.

In practice, I think the biggest need is for test environments on local LAMP & WAMP servers. Let's Encrypt doesn't do this, actually:
https://community.letsencrypt.org/t/can-i-use-letsencrypt-in-localhost/21741

There are pretty complex workarounds, here is one:
https://gist.github.com/andrewn/b30375074398ab94361dc607ac7c0e12

But the bottom line is that for testing, I think that most folks will use self-signed & ignore the warning. Do a quick search and this is still the predominant method recommended out there for testing locally.

Contributor

sbulen commented Oct 29, 2017

I really think it should be up to the admin, and that we should not prohibit the admin from doing so.

In practice, I think the biggest need is for test environments on local LAMP & WAMP servers. Let's Encrypt doesn't do this, actually:
https://community.letsencrypt.org/t/can-i-use-letsencrypt-in-localhost/21741

There are pretty complex workarounds, here is one:
https://gist.github.com/andrewn/b30375074398ab94361dc607ac7c0e12

But the bottom line is that for testing, I think that most folks will use self-signed & ignore the warning. Do a quick search and this is still the predominant method recommended out there for testing locally.

@tinoest

This comment has been minimized.

Show comment
Hide comment
@tinoest

tinoest Oct 29, 2017

Contributor

Then as @albertlast said it should be a check box to enable this functionality. Not the decision being taken away from them.

Contributor

tinoest commented Oct 29, 2017

Then as @albertlast said it should be a check box to enable this functionality. Not the decision being taken away from them.

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Oct 29, 2017

Contributor

The admin already has options to enable https - it is already under admin control.

Contributor

sbulen commented Oct 29, 2017

The admin already has options to enable https - it is already under admin control.

@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Oct 29, 2017

Collaborator

we don't talk about >https< we talk about self signed certs.

Collaborator

albertlast commented Oct 29, 2017

we don't talk about >https< we talk about self signed certs.

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Oct 29, 2017

Contributor

I don't believe adding new options adds any additional protection. It just makes the UI more complex.

I favor simplicity & choice over complexity & constraints. It's a different design approach. Legitimately differing opinions.

Contributor

sbulen commented Oct 29, 2017

I don't believe adding new options adds any additional protection. It just makes the UI more complex.

I favor simplicity & choice over complexity & constraints. It's a different design approach. Legitimately differing opinions.

@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Oct 29, 2017

Collaborator

than keep the feature out,
because you and we already mention this is only intresting for dev and
the most user/admin smf are not dev.

Collaborator

albertlast commented Oct 29, 2017

than keep the feature out,
because you and we already mention this is only intresting for dev and
the most user/admin smf are not dev.

@tinoest

This comment has been minimized.

Show comment
Hide comment
@tinoest

tinoest Oct 29, 2017

Contributor

But you’re making it more complex as you’ll get questions on I have a cert but I’m still getting a error in x browser.

They should have a option to enable that functionality for dev or testing purposes.

Contributor

tinoest commented Oct 29, 2017

But you’re making it more complex as you’ll get questions on I have a cert but I’m still getting a error in x browser.

They should have a option to enable that functionality for dev or testing purposes.

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Oct 29, 2017

Contributor

But that will leave an unnecessary constraint. Again, I believe in giving the admins the freedom to choose these things. We really shouldn't make it more complex than necessary.

Contributor

sbulen commented Oct 29, 2017

But that will leave an unnecessary constraint. Again, I believe in giving the admins the freedom to choose these things. We really shouldn't make it more complex than necessary.

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Oct 29, 2017

Contributor

What might be a good longer-term thing, maybe a 3.0 topic, may be a 'test' vs 'prod' setting or settings. But SMF makes no such distinction today in the 2.0 branch that I know of.

Contributor

sbulen commented Oct 29, 2017

What might be a good longer-term thing, maybe a 3.0 topic, may be a 'test' vs 'prod' setting or settings. But SMF makes no such distinction today in the 2.0 branch that I know of.

@tinoest

This comment has been minimized.

Show comment
Hide comment
@tinoest

tinoest Oct 29, 2017

Contributor

I don’t see how that isn’t giving them the freedom. If anything it is making them more aware of their choice.

I’m just wary of it causing support questions which could be avoided.

Contributor

tinoest commented Oct 29, 2017

I don’t see how that isn’t giving them the freedom. If anything it is making them more aware of their choice.

I’m just wary of it causing support questions which could be avoided.

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Oct 29, 2017

Contributor

Understood, & I agree it's a valid point.

I'm more concerned, however, on support calls from folks not understanding the options, selecting them unwisely, not aware of the cross dependencies, or not being able to test.

When there are differing opinions, I defer to the dev team as the final arbiters of consistency & usability; that's their role.

Contributor

sbulen commented Oct 29, 2017

Understood, & I agree it's a valid point.

I'm more concerned, however, on support calls from folks not understanding the options, selecting them unwisely, not aware of the cross dependencies, or not being able to test.

When there are differing opinions, I defer to the dev team as the final arbiters of consistency & usability; that's their role.

@jdarwood007

This comment has been minimized.

Show comment
Hide comment
@jdarwood007

jdarwood007 Oct 29, 2017

Member

For now, I would suggest tying this into !empty($db_show_debug). If that is set, we are most likely in a dev environment as it is and a self signed would be fine. This doesn't make it a feature, but makes it function in a developers local environment for testing purposes. In future versions we can take on a better solution.

Member

jdarwood007 commented Oct 29, 2017

For now, I would suggest tying this into !empty($db_show_debug). If that is set, we are most likely in a dev environment as it is and a self signed would be fine. This doesn't make it a feature, but makes it function in a developers local environment for testing purposes. In future versions we can take on a better solution.

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Oct 29, 2017

Contributor

That makes sense.

Contributor

sbulen commented Oct 29, 2017

That makes sense.

Allow self-signed if db_show_debug enabled
Signed by Shawn Bulen, bulens@pacbell.net
@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Oct 29, 2017

Contributor

Change made. It's a little awkward, but it does work.
I'm about to update the PR description with some usage notes.

Contributor

sbulen commented Oct 29, 2017

Change made. It's a little awkward, but it does work.
I'm about to update the PR description with some usage notes.

@colinschoen

This comment has been minimized.

Show comment
Hide comment
@colinschoen

colinschoen Nov 2, 2017

Member

Grr I really don't like polluting $db_show_debug as this has nothing to do with what that setting's name implies. Can we think of a better setting to use to enable this? This seems like a nontrivial side effect.

Member

colinschoen commented Nov 2, 2017

Grr I really don't like polluting $db_show_debug as this has nothing to do with what that setting's name implies. Can we think of a better setting to use to enable this? This seems like a nontrivial side effect.

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Nov 2, 2017

Contributor

I am not aware of any other test/prod setting.

As noted above, i dont think we should restrict usage of self-signed certs. If an admin wants to uae one, s/he should be allowed to do so. I'd rather leave the PR as initially submitted.

When I initially added this cert check, I was solving the problem of sites basically disappearing when set to https without a cert. I can imagine plenty of prod uses for self-signed: small private clubs, teams, faculties, etc, where self-signed meets their needs.

Contributor

sbulen commented Nov 2, 2017

I am not aware of any other test/prod setting.

As noted above, i dont think we should restrict usage of self-signed certs. If an admin wants to uae one, s/he should be allowed to do so. I'd rather leave the PR as initially submitted.

When I initially added this cert check, I was solving the problem of sites basically disappearing when set to https without a cert. I can imagine plenty of prod uses for self-signed: small private clubs, teams, faculties, etc, where self-signed meets their needs.

@colinschoen

This comment has been minimized.

Show comment
Hide comment
@colinschoen

colinschoen Nov 2, 2017

Member

I’m leaning towards allowing self-signed in any context.

Member

colinschoen commented Nov 2, 2017

I’m leaning towards allowing self-signed in any context.

@tinoest

This comment has been minimized.

Show comment
Hide comment
@tinoest

tinoest Nov 2, 2017

Contributor

I can’t think of any reason to use a self signed cert when it is so easy to generate a valid one. If you’re not comfortable with letsencrypt and the command line then use;

https://www.sslforfree.com/

It’s not good practice to use self signed certificates anymore, they are susceptible to man in the middle attacks as anyone can issue a similar cert and google will not increase your page rankings either for having a https site.

I get it on testing ( kinda ) but really don’t think it’s a good idea.

Contributor

tinoest commented Nov 2, 2017

I can’t think of any reason to use a self signed cert when it is so easy to generate a valid one. If you’re not comfortable with letsencrypt and the command line then use;

https://www.sslforfree.com/

It’s not good practice to use self signed certificates anymore, they are susceptible to man in the middle attacks as anyone can issue a similar cert and google will not increase your page rankings either for having a https site.

I get it on testing ( kinda ) but really don’t think it’s a good idea.

@colinschoen

This comment has been minimized.

Show comment
Hide comment
@colinschoen

colinschoen Nov 2, 2017

Member

The thing is that anyone who knows how to use a self-signed certificate probably knows about the pros and cons of using one. I don't think we should restrict forum administrators from setting up a self-signed cert should they have a specific case where it is useful for them.

Member

colinschoen commented Nov 2, 2017

The thing is that anyone who knows how to use a self-signed certificate probably knows about the pros and cons of using one. I don't think we should restrict forum administrators from setting up a self-signed cert should they have a specific case where it is useful for them.

@colinschoen

This comment has been minimized.

Show comment
Hide comment
@colinschoen

colinschoen Nov 3, 2017

Member

@sbulen Can we go back to having this enabled by default and not hiding it behind $db_show_debug

Member

colinschoen commented Nov 3, 2017

@sbulen Can we go back to having this enabled by default and not hiding it behind $db_show_debug

@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Nov 3, 2017

Collaborator

@colinschoen the basic of https is not only to encrypt the connection,
is also important to know that you connected to the right "server".
when you allow self signed cert than not only a admin can use a server with self signed,
also "bad person" with a man in the middle attack with a self signed cert.

https://security.stackexchange.com/questions/8110/what-are-the-risks-of-self-signing-a-certificate-for-ssl

Ther is no good reason to allow them in general,
when you don't want to check for dev,
than keep the feature out but not lower the default setting of smf.

Collaborator

albertlast commented Nov 3, 2017

@colinschoen the basic of https is not only to encrypt the connection,
is also important to know that you connected to the right "server".
when you allow self signed cert than not only a admin can use a server with self signed,
also "bad person" with a man in the middle attack with a self signed cert.

https://security.stackexchange.com/questions/8110/what-are-the-risks-of-self-signing-a-certificate-for-ssl

Ther is no good reason to allow them in general,
when you don't want to check for dev,
than keep the feature out but not lower the default setting of smf.

Allow use of self-signed
Signed by Shawn Bulen, bulens@pacbell.net
@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Nov 3, 2017

Contributor

Updated, per request.

Contributor

sbulen commented Nov 3, 2017

Updated, per request.

@colinschoen

LGTM approval

After discussion and consideration it is decided to allow self-signed certificates based on the fact that browsers actively warn users if a certificate is not signed by a trusted certificate authority and the subset of forum owners who know how to set up a self signed certificate also are familiar with the risks associated with it. Furthermore, many other similar projects permit self-signed certificates to be used with their software (PHPBB, Owncloud, Wordpress...).

Thanks @sbulen for this PR.

@colinschoen colinschoen merged commit feafe2e into SimpleMachines:release-2.1 Nov 5, 2017

2 checks passed

Scrutinizer No new issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sbulen sbulen deleted the sbulen:allow_self_signed branch Nov 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment