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

Improve HTTPS cert validation for appstore #9054

Closed
georgehrke opened this issue Jun 16, 2014 · 19 comments · Fixed by #15195
Closed

Improve HTTPS cert validation for appstore #9054

georgehrke opened this issue Jun 16, 2014 · 19 comments · Fixed by #15195

Comments

@georgehrke
Copy link
Contributor

make code use https interface of appstore and properly validate it's certificate

cc @karlitschek

@georgehrke
Copy link
Contributor Author

related to #8808

@georgehrke georgehrke changed the title Improve for HTTPS cert validation for appstore Improve HTTPS cert validation for appstore Jun 16, 2014
@georgehrke georgehrke self-assigned this Jun 16, 2014
@karlitschek
Copy link
Contributor

yep. will do the next few days

@MorrisJobke MorrisJobke added this to the ownCloud 7 milestone Jun 16, 2014
@georgehrke
Copy link
Contributor Author

@LukasReschke could you do me the favor and evaluate if it's enough to make this https or if there is a need to strengthen Util::getUrlContent()

@PVince81
Copy link
Contributor

Setting to OC 7 CE

@PVince81 PVince81 modified the milestones: ownCloud 7 CE, ownCloud 7 Jun 20, 2014
@georgehrke
Copy link
Contributor Author

ping @LukasReschke

@LukasReschke
Copy link
Member

I'll check this right away. Missed this one. sorry.

@georgehrke
Copy link
Contributor Author

thanks :)

@LukasReschke
Copy link
Member

We need to harden Util::getUrlContent() for this one. See the discussion on #7330 why PHP sucks on this and unfortunately I really didn't have the needed time to harden this as needed.

What has to be done:

  1. Harden getUrlContent() similar as done by myself
  2. Add a parameter verifyCert to getUrlContent (defaulting to false since PHP is really bad with SSL and this will break other stuff otherwise)
  3. Add a cafile parameter which allows to use a custom CA file since using the system keystore in PHP is unreliable.
  4. Package the publickey with ownCloud.
  5. Set SNI_enabled to true (http://php.net/manual/en/context.ssl.php)
  6. Set disable_compression to true when available (http://php.net/manual/en/context.ssl.php)
  7. Write extensive unit tests ;-)

This requires proper testing on different PHP versions and with/without cURL installed.
Is this something you could take care of? That would be awesome!

\cc @karlitschek

@georgehrke
Copy link
Contributor Author

This requires proper testing on different PHP versions and with/without cURL installed.
Is this something you could take care of? That would be awesome!

Isn't it enough to implement it, write a test and let jenkins take care of testing in different environments?

@LukasReschke
Copy link
Member

Isn't it enough to implement it, write a test and let jenkins take care of testing in different environments?

No. Different PHP versions have an absolutely different behaviour regarding SSL, that's why we need Travis ;-) \cc @DeepDiver1975
Additionally we have to support cURL and plain PHP, oC will automatically use cURL if available (I guess the PHP one won't run on Jenkins therefore).
What would be possible is to refactor this into two different functions and write unit tests for both then :-)

@PVince81
Copy link
Contributor

PVince81 commented Jul 7, 2014

I wonder: are there that many environments where mod_curl is not available ? Do we still need to support that as well ?

@karlitschek
Copy link
Contributor

@PVince81 Unfortunately we have to support this too. Shared hosters don´t have that installed sometimes.

@DeepDiver1975
Copy link
Member

So what is the status here? OC7 is about to leave soon ...

@karlitschek
Copy link
Contributor

will be done in the next 2 days

@MTRichards
Copy link
Contributor

Ping?

@MTRichards
Copy link
Contributor

are you done yet?

@karlitschek
Copy link
Contributor

The SSL cert is done. I can see why an additional hard check against the actuall cert can be useful in some scenarios. But I´m not sure this is a showstopper for the release.

@MorrisJobke
Copy link
Contributor

@karlitschek @craigpg Possible issue for first sprint?

@karlitschek
Copy link
Contributor

actually I´m not sure if this should be very high on our priority list at the moment to be totally honest

@craigpg craigpg modified the milestones: Backlog, ownCloud 7 CE Sep 2, 2014
@craigpg craigpg modified the milestones: ownCloud 7 backlog, backlog Sep 17, 2014
@craigpg craigpg assigned karlitschek and unassigned georgehrke Sep 17, 2014
@DeepDiver1975 DeepDiver1975 modified the milestones: ownCloud 7 backlog, backlog Jan 8, 2015
@MorrisJobke MorrisJobke modified the milestones: 8.1-current, backlog Mar 26, 2015
@lock lock bot locked as resolved and limited conversation to collaborators Aug 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.