#1194 client: use default CA store during SSL auth if serverca is unset
Merged by tkopecek. Opened by ktdreyer.
ktdreyer/koji ssl-ca-fallback  into  master

Download 1194.patch

Prior to this change, if the following conditions were true:

1) A Koji client uses SSL authentication,
2) The user does not set the "serverca" option for their profile,
3) The user does not have a "~/.koji/serverca.crt" file present,

Then Koji did not use the default CA store to validate the hub's CA.
read_defaults() was setting the "serverca" value to an empty string ""
in these cases. This led to an AuthError in ssl_login() for the empty
string:

raise AuthError("Server CA %s doesn't exist or is not accessible" % serverca)

~~Simplify read_defaults() to only expand "cert" and "serverca" if the default files exist.~~

~~With this change, the SSL authentication code will fall back to the system-wide CA bundle instead of raising AuthError in ssl_login(). This matches the behavior with GSSAPI authentication.~~

EDIT after discussion below:
Update ssl_login() to ignore empty strings in this case, treating them the same as None.

The code in _sendOneCall() already checks this value in a similar way, with "if verify:", so we're matching that behavior here.

With this change, Koji clients will fall back to using the default CA store.

:thumbsup:

+1 sounds good!

Could we not fix this simply by:

-        if serverca is not None and not os.access(serverca, os.R_OK):
+        if serverca and not os.access(serverca, os.R_OK):

which gives the side benefit that user config can set serverca to empty to unset a global serverca setting

@tkopecek who is the ping for?

https://pagure.io/koji/issue/1568

Ouch, for @ktdreyer - does it solve you problem?

@ktdreyer?

rebased onto a939a3d2c2f04e77a5adf2316dc4137bc9204f19

Sorry for the delay. I finally got around to testing @mikem's idea today and re-reading the serverca code.

I tested Mike's patch in a local environment and it works great. I've amended this PR and re-pushed with an updated commit message.

@ktdreyer I've extended it a bit in #1989 - do you find it better, or is this sufficient?

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

I've dropped mine, let's use this.

:thumbsup:

Commit 1dd5e665 fixes this pull-request

Pull-Request has been merged by tkopecek

Metadata Update from @jcupova:
- Pull-request tagged with: testing-done

Metadata