Bug 886525 - if insecure=True do not validate peer certificate
Summary: if insecure=True do not validate peer certificate
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise Virtualization Manager
Classification: Red Hat
Component: ovirt-engine-iso-uploader
Version: 3.1.0
Hardware: Unspecified
OS: Unspecified
urgent
unspecified
Target Milestone: ---
: 3.2.0
Assignee: Alex Lourie
QA Contact: Ilanit Stein
URL:
Whiteboard: integration
Depends On: 915225 915231
Blocks:
TreeView+ depends on / blocked
 
Reported: 2012-12-12 13:42 UTC by Petr Spacek
Modified: 2015-09-22 13:09 UTC (History)
17 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Previously, the --insecure flag was being passed to the rhevm-iso-uploader with the intention of operating without a security certificate. However, the --insecure flag removes certification validation, rather than the requirement of a certificate. Users that tried to upload ISOs using the rhevm-iso-uploader by passing the --insecure flag found that they could not upload ISO images without a CA certificate. A new flag was created in ovirt-sdk allowing insecure object creation. The iso-uploader uses the new ovirt-sdk and now --insecure option no longer requireds a CA certificate.
Clone Of:
: 915225 (view as bug list)
Environment:
Last Closed: 2013-06-10 20:17:21 UTC
oVirt Team: ---
Target Upstream Version:


Attachments (Terms of Use)
curl-test.c (1.54 KB, text/x-csrc)
2013-02-05 16:01 UTC, Alon Bar-Lev
no flags Details


Links
System ID Private Priority Status Summary Last Updated
Red Hat Product Errata RHBA-2013:0919 0 normal SHIPPED_LIVE Updated package: rhevm-iso-uploader 2013-06-11 00:14:38 UTC
oVirt gerrit 10815 0 None MERGED packaging: Fixed creating objects in an insecure way 2020-12-17 01:36:26 UTC

Description Petr Spacek 2012-12-12 13:42:19 UTC
Description of problem:
Rhevm-iso-uploader ignores --insecure option and still looks for CA certificate.


Version-Release number of selected component (if applicable):
rhevm-iso-uploader-3.1.0-8.el6ev.noarch


How reproducible:
100 %


Steps to Reproduce:
1. configure /etc/ovirt-engine/isouploader.conf with domain, host and user name
2. run "rhevm-iso-uploader --insecure list" on machine where CA cert is not present in default path /etc/pki/ovirt-engine/ca.pem
  

Actual results:
# rhevm-iso-uploader --insecure list
Please provide the REST API password for the admin@internal RHEV-M user (CTRL+D to abort): 
ERROR: Problem connecting to the REST API.  Is the service available and does the CA certificate exist?


Expected results:
Command will do the job even without the certificate :-)


Additional info:
1) ISO uploader still looks for CA certificate as proved by strace:
# strace -f -v rhevm-iso-uploader --insecure list

<snip>

open("/etc/pki/ovirt-engine/ca.pem", O_RDONLY) = -1 ENOENT (No such file or directory)
close(4)                                = 0
stat("/usr/share/locale/en_US.UTF8/LC_MESSAGES/rhevm-iso-uploader.mo", 0x7fff3840f510) = -1 ENOENT (No such file or directory)
stat("/usr/share/locale/en_US/LC_MESSAGES/rhevm-iso-uploader.mo", 0x7fff3840f510) = -1 ENOENT (No such file or directory)
stat("/usr/share/locale/en.UTF8/LC_MESSAGES/rhevm-iso-uploader.mo", 0x7fff3840f510) = -1 ENOENT (No such file or directory)
stat("/usr/share/locale/en/LC_MESSAGES/rhevm-iso-uploader.mo", 0x7fff3840f510) = -1 ENOENT (No such file or directory)
stat("/etc/localtime", {st_dev=makedev(253, 0), st_ino=836, st_mode=S_IFREG|0644, st_nlink=1, st_uid=0, st_gid=0, st_blksize=4096, st_blocks=8, st_size=2246, st_atime=2012/12/12-14:01:01, st_mtime=2012/09/03-21:16:49, st_ctime=2012/09/14-14:40:32}) = 0
fstat(3, {st_dev=makedev(253, 0), st_ino=134559, st_mode=S_IFREG|0644, st_nlink=1, st_uid=0, st_gid=0, st_blksize=4096, st_blocks=0, st_size=0, st_atime=2012/12/12-14:06:16, st_mtime=2012/12/12-14:39:03, st_ctime=2012/12/12-14:39:03}) = 0
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f66553e2000
write(3, "2012-12-12 14:39:08::ERROR::rhev"..., 157) = 157
write(2, "ERROR: Problem connecting to the"..., 104ERROR: Problem connecting to the REST API.  Is the service available and does the CA certificate exist?
) = 104
close(3)                                = 0
munmap(0x7f66553e2000, 4096)            = 0
rt_sigaction(SIGINT, {SIG_DFL, [], SA_RESTORER, 0x321ba0f500}, {0x321df0db70, [], SA_RESTORER, 0x321ba0f500}, 8) = 0
exit_group(0)                           = ?


2) Error messages should be clearer, as stated in bug 876554. IMHO connection error and certificate error should be clearly distinguished.

Comment 1 Alex Lourie 2013-01-01 15:17:19 UTC
The bug is actually in SDK:

If we provide ca_file param during the API object initialization, the "--insecure" option is ignored.

Comment 2 Michael Pasternak 2013-01-02 10:52:02 UTC
(In reply to comment #1)
> The bug is actually in SDK:
> 
> If we provide ca_file param during the API object initialization, the
> "--insecure" option is ignored.

this is not correct, --insecure in sdk used to disable certificate requirement,
if it specified, --insecure is irrelevant, also afaiu the problem reported by Petr is root different.

Comment 3 Alex Lourie 2013-01-09 11:01:10 UTC
Michael

The problem specified by Petr is exactly that - the --ignore option is not "used" by the iso-uploader tool. It is not honored because if it is supplied, we initialize the API object with insecure=True. But, if also ca_file is provided, then the "insecure" option is ignored by the SDK.

That's why I'm saying that the problem is in SDK. At the very least it is inconsistent - if ca_file is provided, the "insecure" is ignored, but on the other hand, if ca_file is not provided, I must supply "insecure" option.

I think that "insecure" option, if provided, should be honored by the SDK. If you don't want it, just make it work "insecurely" if there's no ca_file provided.

Comment 4 Michael Pasternak 2013-01-09 11:21:25 UTC
Alex,

(In reply to comment #3)
> Michael
> 
> The problem specified by Petr is exactly that - the --ignore option is not
> "used" by the iso-uploader tool. It is not honored because if it is
> supplied, we initialize the API object with insecure=True. But, if also
> ca_file is provided, then the "insecure" option is ignored by the SDK.
> 
> That's why I'm saying that the problem is in SDK. 

please read again Comment 2, there is no problem in sdk, --insecure option used to tell sdk that it should not ask for certificate, but if user provides certificate, there is no point of asking sdk to ignore it ...

> I think that "insecure" option, if provided, should be honored by the SDK.
> If you don't want it, just make it work "insecurely" if there's no ca_file
> provided.

--insecure exist for one reason - to make user understand that server identity will not be validated and explicitly approve that by passing this flag, 

implicitly allowing this flow (by not mandating certificate) is wrong as it 
may lead to security issues.,

i suggest you closing this as not-a-bug.

Comment 5 Michael Pasternak 2013-01-09 11:26:05 UTC
(In reply to comment #4)
> 
> i suggest you closing this as not-a-bug.

wait,

the original description says:

"Rhevm-iso-uploader ignores --insecure option and still looks for CA certificate."

so as i read this:

1. iso-uploaded does not pass --insecure to sdk and not
like you've understood this bug "that --insecure should ignore specified CA"

2. from "ERROR: Problem connecting to the REST API.  Is the service available and does the CA certificate exist?" is not clear that this bug related to certificate at all, hope you not just catching exception there ...

Comment 6 Alex Lourie 2013-01-09 12:52:10 UTC
OK, got it.

Then the fix must come in the iso-uploader.

Comment 7 Alex Lourie 2013-01-09 15:22:58 UTC
The patch is uploaded for review: http://gerrit.ovirt.org/#/c/10815/

Comment 8 Alex Lourie 2013-01-16 10:14:51 UTC
After the discussion, we believe that the problem is in SDK, as it doesn't behave in an expected way.

Cited from the discussion on the above patch:

The following notation is *CORRECT* and expected to work correctly:
 self.api = API(
     url=url,
     username=self.configuration.get("user"),
     password=self.configuration.get("passwd"),
     ca_file=self.configuration.get("engine_ca"),
     insecure=self.configuration.get("insecure", False),
 )

If it does not, there is a bug in the API, which refer to SSL/TLS parameters even if insecure is False.
What we need is to validate that the API indeed behaves incorrectly and fix the API.

Michael?

Comment 10 Michael Pasternak 2013-01-16 11:31:50 UTC
(In reply to comment #8)
> After the discussion, we believe that the problem is in SDK, as it doesn't
> behave in an expected way.
> 
> Cited from the discussion on the above patch:
> 
> The following notation is *CORRECT* and expected to work correctly:
>  self.api = API(
>      url=url,
>      username=self.configuration.get("user"),
>      password=self.configuration.get("passwd"),
>      ca_file=self.configuration.get("engine_ca"),
>      insecure=self.configuration.get("insecure", False),
>  )
> 
> If it does not, there is a bug in the API, which refer to SSL/TLS parameters
> even if insecure is False.
> What we need is to validate that the API indeed behaves incorrectly and fix
> the API.
> 
> Michael?

Alex,

You're taking wrong path again,

about sdk
=========

1. if insecure is False & no CA cert provided, will be raised error.
2. if insecure is True & no CA cert provided, no error will be raised.
3. if insecure is True & CA cert is provided, will be used CA cert

all three by-design, for more info see Comment 4.

about this bug
==============

Petr reported that when he trying to connect with --insecure and no CA cert., he get this error:

"ERROR: Problem connecting to the REST API.  Is the service available and does the CA certificate exist?"

according to the message and without seeing the code, i assume that you're catching generic Exception in your code and guessing what the cause is, by
returning generic error, so actually the cause can be completely not related to the --insecure flag,

for instance Petr can specify url using http:// protocol while server running HTTPS,

correct behaviour in iso-uploader is catching specific exceptions and act accordingly rather than catching generic Exception and making assumptions,
by generic error,

currently thrown exceptions by SDK are:

1. NoCertificatesError - when no CA provided and --insecure is False
2. UnsecuredConnectionAttemptError - when HTTP protocol is used while 
   server running HTTPS
3. ImmutableError - on sdk < 3.2 is raised when sdk initiation attempt
   occurred when sdk instance already exist under the same domain.
4. DisconnectedError - when you trying to use sdk after it was explicitly 
   disconnected
5. MissingParametersError - is raised when you trying to use get() method 
   without providing id or name.
6. RequestError - any kind of oVirt server error
7. ConnectionError - raised when transport layer error ocured

also i suggest at least printing exception message when you do not
know exception type, and not internal/generic errors.

Comment 11 Alon Bar-Lev 2013-01-16 16:22:57 UTC
(In reply to comment #10)
> 
> Alex,
> 
> You're taking wrong path again,
> 
> about sdk
> =========
> 
> 1. if insecure is False & no CA cert provided, will be raised error.
> 2. if insecure is True & no CA cert provided, no error will be raised.
> 3. if insecure is True & CA cert is provided, will be used CA cert

This is way to complex.

Should be:

1. If !insecure - check CA
2. If insecure - do not check CA

No 3 options no lookup within passive resource if active resource is provided.

> 
> all three by-design, for more info see Comment 4.

This is a bad design, please fix it.
Should be much simpler...

the insecure parameter is the active resource it determine how we operate.
the existence of other parameters is irrelevant.

Thanks,
Alon.

Comment 12 Alex Lourie 2013-01-16 16:38:54 UTC
> about this bug
> ==============
> 
> Petr reported that when he trying to connect with --insecure and no CA
> cert., he get this error:
> 
> "ERROR: Problem connecting to the REST API.  Is the service available and
> does the CA certificate exist?"
> 
> according to the message and without seeing the code, i assume that you're
> catching generic Exception in your code and guessing what the cause is, by
> returning generic error, so actually the cause can be completely not related
> to the --insecure flag,
> 
> for instance Petr can specify url using http:// protocol while server
> running HTTPS,
> 

Michael,

The problem is not in the wrong exception or the error message. The problem is in the fact that
Petr supplied an option called "--insecure" to the tool he ran, which was transferred to the API,
and that action should have caused the SDK NOT to require the certificate. The error message shown
just proves that it is not the case, and that certificate is still being required.

As such, I agree with Alon's comment, that the current behaviour of the SDK is confusing and
overly complex. The "insecure" option must have precedence over the ca_file, and if supplied as True - 
that would mean that user forcefully decided to be insecure, and that should be honoured in the
SDK.


> correct behaviour in iso-uploader is catching specific exceptions and act
> accordingly rather than catching generic Exception and making assumptions,
> by generic error,
> 
>

Comment 13 Michael Pasternak 2013-01-16 17:23:47 UTC
(In reply to comment #11)
> (In reply to comment #10)
> > 
> > Alex,
> > 
> > You're taking wrong path again,
> > 
> > about sdk
> > =========
> > 
> > 1. if insecure is False & no CA cert provided, will be raised error.
> > 2. if insecure is True & no CA cert provided, no error will be raised.
> > 3. if insecure is True & CA cert is provided, will be used CA cert
> 
> This is way to complex.
> 
> Should be:
> 
> 1. If !insecure - check CA
> 2. If insecure - do not check CA

that's exactly how it's works and described in the Comment 10!

> 
> No 3 options no lookup within passive resource if active resource is
> provided.

disagree, if you don't want certificate to be used, - don't pass it!

> 
> > 
> > all three by-design, for more info see Comment 4.
> 
> This is a bad design, please fix it.
> Should be much simpler...
> 
> the insecure parameter is the active resource it determine how we operate.

you are wrong!, --insecure is administrative tool to signal that user
wish working without CA/understand that server identity won't be validated and not used to signal to use certificate or not, - this is how linux tools work.

and once again, this is *NOT* related to this bug!

> the existence of other parameters is irrelevant.
> 
> Thanks,
> Alon.

Comment 14 Michael Pasternak 2013-01-16 17:34:25 UTC
(In reply to comment #12)
> > about this bug
> > ==============
> > 
> > Petr reported that when he trying to connect with --insecure and no CA
> > cert., he get this error:
> > 
> > "ERROR: Problem connecting to the REST API.  Is the service available and
> > does the CA certificate exist?"
> > 
> > according to the message and without seeing the code, i assume that you're
> > catching generic Exception in your code and guessing what the cause is, by
> > returning generic error, so actually the cause can be completely not related
> > to the --insecure flag,
> > 
> > for instance Petr can specify url using http:// protocol while server
> > running HTTPS,
> > 
> 
> Michael,
> 
> The problem is not in the wrong exception or the error message. The problem
> is in the fact that
> Petr supplied an option called "--insecure" to the tool he ran, which was
> transferred to the API,
> and that action should have caused the SDK NOT to require the certificate.
> The error message shown
> just proves that it is not the case, and that certificate is still being
> required.
> 

Alex,

Please stop guessing!, you do *NOT* know that SDK construction failed because
of certificate, there is no possible way that if you passed insecure=True will
raised error, this flow is used by automation tests for about 1/2 year now.

Comment 15 Keith Robertson 2013-01-16 17:47:34 UTC
Michael and Alex,

We have exhaustively discussed this issue here and in [1].  It is my opinion that this issue can be easily resolved with a simple modification to connection.py in the Python oVirt SDK.

Simply do this...
//----- Change this...
        if(u.scheme == 'https'):
            if not insecure and not ca_file:
                raise NoCertificatesError

//----- To This...
        if(u.scheme == 'https'):            
            if not insecure and not ca_file:
                raise NoCertificatesError
            if insecure:
                ca_file=None <--- Null out the CA file.

Currently, if you pass 'insecure=True' and a CA file the HTTPSConnection object will receive a non-null CA file and will attempt validation.  This is an error as the two variables are mutually exclusive.

So the API either needs to throw an exception when this condition occurs *or* it must give preferential treatment to 'insecure=True'.

Finally, this is *not* a bug with the tools.  This is a bug in the API, IMO.

Cheers,
Keith

[1] http://gerrit.ovirt.org/#/c/10815/

Comment 16 Michael Pasternak 2013-01-16 18:08:50 UTC
(In reply to comment #15)
> Michael and Alex,
> 
> We have exhaustively discussed this issue here and in [1].  It is my opinion
> that this issue can be easily resolved with a simple modification to
> connection.py in the Python oVirt SDK.
> 
> Simply do this...
> //----- Change this...
>         if(u.scheme == 'https'):
>             if not insecure and not ca_file:
>                 raise NoCertificatesError
> 
> //----- To This...
>         if(u.scheme == 'https'):            
>             if not insecure and not ca_file:
>                 raise NoCertificatesError
>             if insecure:
>                 ca_file=None <--- Null out the CA file.

Keith,

Thanks for commenting on this, but as i've explained in the previous comment
--insecure is not used to ignore the certificate, but to approve connecting
to SSL site without it, therefore suggested change ain't relevant,

also, bug reporter did *NOT* passed certificate and entire bug is *NOT*
related to the certificate at all, also none complaining on cert., being
used when --insecure is raised,

i just saw your code and apparently you catch NoCertificatesError at line 459,
so it's even prove my theory that you facing some error during init and *NOT*
reporting it, 

also i see in upstream that MASTER code is different then in this bug descr., so maybe Alon already fixed it by [1]

[1] except Exception, e:
                logging.error(
                    _("Unable to connect to REST API.  Message: %s"),
                    e
                )

=> i think you should suggest Petr, using new version of your tool.

Comment 17 Alon Bar-Lev 2013-01-16 19:16:39 UTC
Micahel,

Please fix your code, it is incorrect and unexpected.

If insecure is passed you should ignore peer certificate that's it no more, and no matter if you get more parameters to constructor.

Thanks,

Comment 18 Michael Pasternak 2013-01-16 20:02:46 UTC
(In reply to comment #17)
> Micahel,
> 
> Please fix your code, it is incorrect and unexpected.
> 
> If insecure is passed you should ignore peer certificate that's it no more,
> and no matter if you get more parameters to constructor.

Alon,

did you missed my previous replies on this? --insecure have nothing to do with certificate, it is not used to tell to use certificate or not, the purpose of this flag is to approve connecting to SSL sites without certificate,

the definition of it is: "Allow connections to SSL sites without certificates",
and that's what you should delegate to iso-uploader users,
(btw you can find very same flag/behaviour in 'curl')

as for me i see this discussion as fulfil itself, please take your attention 
to resolving bug mentioned by reporter or advice him upgrading your tool if
you already fixed it.

> 
> Thanks,

Comment 19 Alon Bar-Lev 2013-01-16 20:08:55 UTC
Michael,

Stop repeating your-self, I understand perfectly what you trying to argue and you are wrong.

The problem is at the SDK and your interpretation of PKI and TLS/SSL usage.

As TLS must have peer certificate and you do not support local certificate, then insecure tells one thing: Ignore the validation of the peer certificate.

The problem is within the SDK there is nothing to fix in other tools.

Thanks,
Alon

Comment 20 Michael Pasternak 2013-01-16 20:13:38 UTC
(In reply to comment #19)
> Michael,
> 
> Stop repeating your-self, I understand perfectly what you trying to argue
> and you are wrong.
> 
> The problem is at the SDK and your interpretation of PKI and TLS/SSL usage.
> 
> As TLS must have peer certificate and you do not support local certificate,

not supporting "local certificate"??

> then insecure tells one thing: Ignore the validation of the peer certificate.
> 
> The problem is within the SDK there is nothing to fix in other tools.

what problem alon?

> 
> Thanks,
> Alon

Comment 21 Alon Bar-Lev 2013-01-16 20:20:11 UTC
(In reply to comment #20)
> (In reply to comment #19)
> > Michael,
> > 
> > Stop repeating your-self, I understand perfectly what you trying to argue
> > and you are wrong.
> > 
> > The problem is at the SDK and your interpretation of PKI and TLS/SSL usage.
> > 
> > As TLS must have peer certificate and you do not support local certificate,
> 
> not supporting "local certificate"??

insecure does not effect nor validate nor touch the local certificate, exactly like it should not consider the ca certificate for peer validation.

> 
> > then insecure tells one thing: Ignore the validation of the peer certificate.
> > 
> > The problem is within the SDK there is nothing to fix in other tools.
> 
> what problem alon?

That the following is not treated correctly:

   new API(insecure=True, ca_file='/tmp/ca.crt')

Expected: Do not validate peer certificate, ca_file should be completely ignored.

Comment 22 Michael Pasternak 2013-01-16 20:35:07 UTC
(In reply to comment #21)
> > 
> > what problem alon?
> 
> That the following is not treated correctly:
> 
>    new API(insecure=True, ca_file='/tmp/ca.crt')
> 
> Expected: Do not validate peer certificate, ca_file should be completely
> ignored.

1. apologise, but sdk does not have support for ignoring certificate,
it only has flag making CA certificate requirement to go away,
you can file RFE on this, but i won't ack it as i don't see any point 
in making sdk ignore specific parameter specified by user,
-> don't want it to be used? - don't pass it ...

2. can you explain me how told by you above is related to this bug
and why you're saying " The problem is within the SDK there is nothing to fix in other tools."?

reporter steps to reproduce are:
===============================

1. configure /etc/ovirt-engine/isouploader.conf with domain, host and user name
2. run "rhevm-iso-uploader --insecure list" on machine where CA cert is not present in default path /etc/pki/ovirt-engine/ca.pem

Comment 23 Alon Bar-Lev 2013-01-16 20:42:35 UTC
-> don't want it to be used? - don't pass it ...

This is your mistake.

There are passive and active parameters.
In this case insecure is active and ca_file is passive.
It means that you act upon insecure and if insecure=False you lookup ca_file as ca_file is irrelevant in unsecured mode.

I am not a maintainer of neither SDK nor tools, but as far as I can tell your approach is wrong, and SDK should be fixed to have better and clearer interface.

This is my last comment in this regard, as nothing is progressing.

Thank you.

Comment 24 Alon Bar-Lev 2013-01-16 21:02:08 UTC
Maybe another example will help you to understand.

Let's say we have ~/.ovirt/engine-sdk.conf for defaults.

In this file we can have the defaults of parameters:
Exmaple:
---
user=xxx@local
password=pppp
ca_file=/tmp/ca.crt
---

Now, when API(insecure=True) is used, it is as if user specified API(insecure=True, ca_file='/tmp/ca.crt'), and the expected behavior is to ignore the validity of peer certificate chain in both cases.

The following should also be allowed:
---
user=xxx@local
password=pppp
ca_file=/tmp/ca.crt
unsecured=False
---

While API(insecure=True) is expected to behave exactly as above.

Comment 25 Michael Pasternak 2013-01-16 21:09:42 UTC
(In reply to comment #24)
> Maybe another example will help you to understand.
> 
> Let's say we have ~/.ovirt/engine-sdk.conf for defaults.
> 
> In this file we can have the defaults of parameters:
> Exmaple:
> ---
> user=xxx@local
> password=pppp
> ca_file=/tmp/ca.crt
> ---
> 
> Now, when API(insecure=True) is used, it is as if user specified
> API(insecure=True, ca_file='/tmp/ca.crt'), and the expected behavior is to
> ignore the validity of peer certificate chain in both cases.
> 
> The following should also be allowed:
> ---
> user=xxx@local
> password=pppp
> ca_file=/tmp/ca.crt
> unsecured=False

iso-uploader should encapsulate this by invoking right parameters in sdk
constructor and not forcing sdk to comply with uploader private use-case.

> ---
> 
> While API(insecure=True) is expected to behave exactly as above.

alon,

i'm willing to help, i really do, but all this is just another SDK RFE and not related to this bug,

so our entire discussion is just flooding this bug with not related information,
so let's take it offline.

Comment 26 Alon Bar-Lev 2013-01-16 21:12:01 UTC
It is as there is nothing wrong with the tool.

Comment 27 Michael Pasternak 2013-01-16 21:17:35 UTC
(In reply to comment #26)
> It is as there is nothing wrong with the tool.

how do you know that? do you know what "ERROR: Problem connecting to the REST API.  Is the service available and does the CA certificate exist?" means?

Comment 30 Keith Robertson 2013-01-17 16:33:42 UTC
Mike,

What we are asking for is that the API do some simple parameter validation.  It is our assertion that the parameters 'insecure' and 'ca_cert' should be mutually exclusive. Further, the API should detect this condition and throw an exception.  Parameter validation, as I'm sure you are aware, is quite common particularly in 'public' methods (private methods are not expected to do this).  Since, the API's constructor is clearly public, as a consumer, I expect it to provide some rudimentary parameter validation for the arguments passed in.  

You are asserting that "insecure is not designed to ignore parameters".  By this statement, I would infer that you think the condition 'insecure=True' and 'ca_file!=None' is not an error.  I think this is both wrong and confusing to consumers of the API as you are giving priority to the 'ca_file' and attempting to do server validation.

There are a great many ways to solve this problem but, I think that it is clear from the traffic on the list(s) and the "vigorous" debate in this BZ that there is probably room for improvement in the API's handling of this condition and/or characterization of 'insecure'.

Points:
- insecure: This is a an overly *broad* parameter IMO and I wish it were renamed/removed.  If I supply insecure does it mean that an https URI becomes HTTP? Does it mean that I don't need to supply client certs?  In reality, it simply is a mechanism to allow the client to disable server validation *but* I also cannot also supply a 'ca_file'.  IMO, if I set 'insecure=True' then the API should not attempt to do server validation even if I supply a 'ca_file'.  

- Degrees of security: It would appear to me that the HTTPSConnection you have created in connection.py desires, by default, to perform the *highest* degree of security (ie. SSL + Client validation + Server Validation).  I would argue that the API should take a different stance on this.  For HTTPSConnections, it should be structured in a manner that prefers simple HTTPSConnections with no client or server validation.  It should be up to the caller to *decide* if they want the higher security levels and to supply arguments necessary to make that happen.  I feel that this is an appropriate compromise because the WebApp isn't configured in an ultra-high security mode by default (eg. it doesn't force client validation AFAIK). 

My 2c.  Cheers,
Keith

Comment 31 Michael Pasternak 2013-01-17 17:51:38 UTC
Keith,

(In reply to comment #30)
> Mike,
> 
> There are a great many ways to solve this problem but, I think that it is
> clear from the traffic on the list(s) and the "vigorous" debate in this BZ
> that there is probably room for improvement in the API's handling of this
> condition and/or characterization of 'insecure'.

this traffic is generated because of misunderstanding of the goal of
'insecure' parameter in sdk, i'll try to make it clear as possible this
time, 

it is:
=====

"i *understand* that server identity will not be validated if i will not
supply CA certificate", or in other words "don't tell me what to do",
and as such, 'insecure' cannot dictate sdk how to handle security flows
simply because its meaning is => do-not-throw-error-when-no-cert-on-ssl

to justify my objection of giving 'insecure' meaning that you and alon are
suggesting, i'll give just one example:

sdk outside about 1y+, up until now a lot of people wrote theirs code against
it, and as they aware that [insecure = do-not-throw-error-when-no-cert-on-ssl],
they simply turned off this warning, and one time they providing certificate and another one - no (but told sdk => insecure=True i.e "don't tell me what to do"),

now if i would make (insecure=True) not passing certificate that provided by
users, the server identity will never be validated for them despite that they *sure* that *it is* *validated*, what will cause giant security breach for them.

in security world exist one "gold rule" called "white list", i.e everything
is "black listed" until you explicitly add it to the "white list", same in my case, => validate server until user explicitly *does not pass* the certificate & signals that he knows what he is doing by raising [insecure = do-not-throw-error-when-no-cert-on-ssl] flag,

therefore i will not use any heuristics in sdk to understand  what user has meant rather than let user to do this by himself ....

now you can argue about the name etc., but this is completely different story.

Comment 32 Keith Robertson 2013-02-05 13:30:44 UTC
(In reply to comment #31)
> Keith,
> 
> (In reply to comment #30)
> > Mike,
> > 
> > There are a great many ways to solve this problem but, I think that it is
> > clear from the traffic on the list(s) and the "vigorous" debate in this BZ
> > that there is probably room for improvement in the API's handling of this
> > condition and/or characterization of 'insecure'.
> 
> this traffic is generated because of misunderstanding of the goal of
> 'insecure' parameter in sdk, i'll try to make it clear as possible this
> time, 
> 
> it is:
> =====
> 
> "i *understand* that server identity will not be validated if i will not
> supply CA certificate",

That is a very clunky statement and one which I disagree with.  How about:
- ERROR: Insecure and ca_cert are mutually exclusive.  Please try again.


> 
> sdk outside about 1y+, up until now a lot of people wrote theirs code against
> it, and as they aware that [insecure =
> do-not-throw-error-when-no-cert-on-ssl],

So we are basing all current and future design decisions on, possibly, some poor design choices in the past?  Shouldn't we 'obosolete' that understanding and endeavor to produce a new understanding that is clearer?


> in security world exist one "gold rule" called "white list", i.e everything
> is "black listed" until you explicitly add it to the "white list", same in
> my case, => validate server until user explicitly *does not pass* the
> certificate & signals that he knows what he is doing by raising [insecure =
> do-not-throw-error-when-no-cert-on-ssl] flag,

You are conflating an API *usability* design decision with a well known security policy.  The usability of your API has nothing to do with this security policy and I would argue that you could still remain true to the spirit of that security policy and have a clearer constructor/implementation.

Comment 33 Michael Pasternak 2013-02-05 13:49:18 UTC
(In reply to comment #32)
> (In reply to comment #31)
> > Keith,
> > 
> > (In reply to comment #30)
> > > Mike,
> > > 
> > > There are a great many ways to solve this problem but, I think that it is
> > > clear from the traffic on the list(s) and the "vigorous" debate in this BZ
> > > that there is probably room for improvement in the API's handling of this
> > > condition and/or characterization of 'insecure'.
> > 
> > this traffic is generated because of misunderstanding of the goal of
> > 'insecure' parameter in sdk, i'll try to make it clear as possible this
> > time, 
> > 
> > it is:
> > =====
> > 
> > "i *understand* that server identity will not be validated if i will not
> > supply CA certificate",
> 
> That is a very clunky statement and one which I disagree with.  How about:
> - ERROR: Insecure and ca_cert are mutually exclusive.  Please try again.

i don't mind taking this path, but this won't work for your use-case,
cause Alon/you want to be able specifying --insecure and CA certificate all together, just that SDK in this case should ignore the certificate ....

> 
> 
> > 
> > sdk outside about 1y+, up until now a lot of people wrote theirs code against
> > it, and as they aware that [insecure =
> > do-not-throw-error-when-no-cert-on-ssl],
> 
> So we are basing all current and future design decisions on, possibly, some
> poor design choices in the past?  Shouldn't we 'obosolete' that
> understanding and endeavor to produce a new understanding that is clearer?

i knew you will say this Keith and this statement not fair to say the least, 
i was asked to *warn* when no CA certificate is passed and that's what i'm
doing in SDK, but with possibility to turn off this warning via 'insecure=True' flag,

1. what you're asking for is completely different (new) feature and my opinion
   that adding parameter --x to ignore --y is redundant, - don't pass --y if
   you don't want it to be used.

2. in api (especially in security related apis) there is no such thing as
   giving new understanding to parameters - just trust my years of experience

Comment 34 Keith Robertson 2013-02-05 15:38:25 UTC
(In reply to comment #33)
> (In reply to comment #32)
> > (In reply to comment #31)

> > 
> > That is a very clunky statement and one which I disagree with.  How about:
> > - ERROR: Insecure and ca_cert are mutually exclusive.  Please try again.
> 
> i don't mind taking this path, but this won't work for your use-case,
> cause Alon/you want to be able specifying --insecure and CA certificate all
> together, just that SDK in this case should ignore the certificate ....

I would rather the tool tool fail with an immediate and appropriate error from the API explaining the inconsistency.  Further, if the tool is used with both options being set then I consider that a configuration/usage issue.



> 
> i knew you will say this Keith and this statement not fair to say the least, 
> i was asked to *warn* when no CA certificate is passed and that's what i'm
> doing in SDK, but with possibility to turn off this warning via
> 'insecure=True' flag,

Fair?  I had nothing to do with that request.  I am simply pointing out the ramifications of that choice.

> 
> 1. what you're asking for is completely different (new) feature and my
> opinion
>    that adding parameter --x to ignore --y is redundant, - don't pass --y if
>    you don't want it to be used.
> 
> 2. in api (especially in security related apis) there is no such thing as
>    giving new understanding to parameters - just trust my years of experience

I have many years of experience also and I would say that there is a clear precedent for obsoletion in a great many languages/API's/toolkits.

Comment 35 Alon Bar-Lev 2013-02-05 16:01:36 UTC
Created attachment 693458 [details]
curl-test.c

You wrote that curl is the model, right?

So I constructed a little program using curl api.

You can test it out:

$ gcc curl-test.c -o curl-test -lcurl

$ ./curl-test -h
usage: ./a.out [options] url
    -u - unsecured.
    -c - ca certificate.
    -h - usage.

Now, lets see how curl behaves:

$ ./curl-test -u -c /etc/ssl/certs/DST_ACES_CA_X6.pem https://10.35.0.72
<success>

$ ./curl-test -u -c /tmp/dummy-not-exist-file https://10.35.0.72
<success>

$ ./curl-test -c /etc/ssl/certs/DST_ACES_CA_X6.pem https://10.35.0.72
curl_easy_perform error 60

$ ./curl-test -c /tmp/dsfdsfsfs https://10.35.0.72
curl_easy_perform error 77

Notice that the -u causes ignore of -c, it event not being accessed, which is what I expect.

Comment 36 Michael Pasternak 2013-02-05 16:53:44 UTC
(In reply to comment #34)
> 
> I have many years of experience also and I would say that there is a clear
> precedent for obsoletion in a great many languages/API's/toolkits.

obsoletion - yes
meaning change - never

Comment 37 Michael Pasternak 2013-02-05 17:31:13 UTC
(In reply to comment #35)
> Created attachment 693458 [details]
> curl-test.c
> 
> You wrote that curl is the model, right?
> 
> So I constructed a little program using curl api.
> 
> You can test it out:
> 
> $ gcc curl-test.c -o curl-test -lcurl
> 
> $ ./curl-test -h
> usage: ./a.out [options] url
>     -u - unsecured.
>     -c - ca certificate.
>     -h - usage.
> 
> Now, lets see how curl behaves:
> 
> $ ./curl-test -u -c /etc/ssl/certs/DST_ACES_CA_X6.pem https://10.35.0.72
> <success>
> 
> $ ./curl-test -u -c /tmp/dummy-not-exist-file https://10.35.0.72
> <success>
> 
> $ ./curl-test -c /etc/ssl/certs/DST_ACES_CA_X6.pem https://10.35.0.72
> curl_easy_perform error 60
> 
> $ ./curl-test -c /tmp/dsfdsfsfs https://10.35.0.72
> curl_easy_perform error 77
> 
> Notice that the -u causes ignore of -c, it event not being accessed, which
> is what I expect.

Alon,

I did very same test referring not existent file that days, here is a line
from my history file:

curl -X GET -u $MYUSER --cert /tmp/nill --insecure $URI

and it is causes

curl: (58) Unable to load certificate /tmp/nill

now when i'm looking in the curl help, i should be using --cacert instead of --cert which is client cert, ca cert ignored indeed when --insecure is raised,

but as you see client certificate still validated regardless of --insecure flag,

i have compromise suggestion, lets:

1. deprecate --insecure
2. file RFE for sdk to add new flag --no-check-certificate (or similar)
3. currently do not pass certs (client/ca) in tools (till migrated to new sdk) 

how does it sounds?

Comment 38 Alon Bar-Lev 2013-02-05 17:49:06 UTC
(In reply to comment #37)
> (In reply to comment #35)
> > Created attachment 693458 [details]
> > curl-test.c
> > 
> > You wrote that curl is the model, right?
> > 
> > So I constructed a little program using curl api.
> > 
> > You can test it out:
> > 
> > $ gcc curl-test.c -o curl-test -lcurl
> > 
> > $ ./curl-test -h
> > usage: ./a.out [options] url
> >     -u - unsecured.
> >     -c - ca certificate.
> >     -h - usage.
> > 
> > Now, lets see how curl behaves:
> > 
> > $ ./curl-test -u -c /etc/ssl/certs/DST_ACES_CA_X6.pem https://10.35.0.72
> > <success>
> > 
> > $ ./curl-test -u -c /tmp/dummy-not-exist-file https://10.35.0.72
> > <success>
> > 
> > $ ./curl-test -c /etc/ssl/certs/DST_ACES_CA_X6.pem https://10.35.0.72
> > curl_easy_perform error 60
> > 
> > $ ./curl-test -c /tmp/dsfdsfsfs https://10.35.0.72
> > curl_easy_perform error 77
> > 
> > Notice that the -u causes ignore of -c, it event not being accessed, which
> > is what I expect.
> 
> Alon,
> 
> I did very same test referring not existent file that days, here is a line
> from my history file:
> 
> curl -X GET -u $MYUSER --cert /tmp/nill --insecure $URI
> 
> and it is causes
> 
> curl: (58) Unable to load certificate /tmp/nill

You are confusing between API and CLI.

Comment 39 Alon Bar-Lev 2013-02-05 17:56:13 UTC
(In reply to comment #37)
> i have compromise suggestion, lets:
> 
> 1. deprecate --insecure

OK, although --no-check-certificate is same as inseure.
I don't care how this parameter of SDK is called.
But should be boolean and when set chain should not be validated at all.

API should be positive not negative, with default, hence:

secured=True - default
check_certificate=True - default
whatever=True - default

Not specify = as if xxx=@DEFAULT@ was specified

Please refer to curl API, the nature of set attribute is something that is well accepted, better than have all in constructor.

> 2. file RFE for sdk to add new flag --no-check-certificate (or similar)

This bug is the bug, no need for RFE, as it is actually an issue to solve.

> 3. currently do not pass certs (client/ca) in tools (till migrated to new
> sdk) 
> 
> how does it sounds?

Great, moving back to sdk.

Thanks.

Comment 40 Alon Bar-Lev 2013-02-05 18:01:48 UTC
(In reply to comment #36)
> (In reply to comment #34)
> > 
> > I have many years of experience also and I would say that there is a clear
> > precedent for obsoletion in a great many languages/API's/toolkits.
> 
> obsoletion - yes
> meaning change - never

Not sure this is a meaning change... as I shown you with the curl API example.

Comment 41 Michael Pasternak 2013-02-05 18:25:19 UTC
(In reply to comment #40)
> (In reply to comment #36)
> > (In reply to comment #34)
> > > 
> > > I have many years of experience also and I would say that there is a clear
> > > precedent for obsoletion in a great many languages/API's/toolkits.
> > 
> > obsoletion - yes
> > meaning change - never
> 
> Not sure this is a meaning change... as I shown you with the curl API
> example.

it is, as insecure parameter meaning in sdk is do-not-throw-error-when-no-ca,
it's only disables warning, and as i explained in Comment 31, i can't change
the meaning of this parameter.

Comment 42 Michael Pasternak 2013-02-05 18:47:36 UTC
(In reply to comment #39)
> Please refer to curl API, the nature of set attribute is something that is
> well accepted, better than have all in constructor.

it's irrelevant, having it in constructor with default is exactly the 
same as defining it as instance/class variable and assigning the default there,
it's just a way to make parameter optional in python (or emulation of constructor/signature overload)

this is common python practice.

> 
> > 2. file RFE for sdk to add new flag --no-check-certificate (or similar)
> 
> This bug is the bug, no need for RFE, as it is actually an issue to solve.

i disagree, this is a bug in your view of things, i was asked to provide 
ability to turn off no-cert warning and that's what i did,

what you are asking for is a different feature.

Comment 43 Barak 2013-02-11 09:22:55 UTC
After thinking and discussing this issue for a long time with several people we have reached the folowing solution:

- A new flag will be added to the SDK  to indicate 
  that a cert chain validation should be performed ("validate-cert-chain").
- The default value should be TRUE
- This parameter will be validated against the cert existence (meaning  validate-
  cert-chain=FALSE will conflict with ca_cert=<not null>)
- This parameter should be also added to the cli and it's configuration file but 
  here with the opposite meaning (--dont-validate-cert-chain) to change the 
  default by raising a flag which is more intuitive. but in the instantiation of 
  the SDK this will be translated to  validate-cert-chain=FALSE
- ovirt-engine-iso-uploader will need to use this flag to resolve the above ]
  conflict.

 
 
Alon, keith, Alex thoughts ?

Comment 44 Alon Bar-Lev 2013-02-12 05:14:03 UTC
(In reply to comment #43)
> - This parameter will be validated against the cert existence (meaning 
> validate-
>   cert-chain=FALSE will conflict with ca_cert=<not null>)

What do you mean conflict?
It should not conflict with anything.
Specifying validate-cert-chain=FALSE should ignore ca_cert.

Just like the example I shown of curl, and I can show many others.

Thanks!

Comment 45 Barak 2013-02-25 09:12:28 UTC
(In reply to comment #44)
> (In reply to comment #43)
> > - This parameter will be validated against the cert existence (meaning 
> > validate-
> >   cert-chain=FALSE will conflict with ca_cert=<not null>)
> 
> What do you mean conflict?
> It should not conflict with anything.
> Specifying validate-cert-chain=FALSE should ignore ca_cert.
> 
> Just like the example I shown of curl, and I can show many others.
> 

agreed

> Thanks!

Comment 46 Alex Lourie 2013-03-05 10:54:07 UTC
The patch is updated accordingly.

Comment 49 Ilanit Stein 2013-04-07 10:48:03 UTC
Verified on sf-13

Comment 50 errata-xmlrpc 2013-06-10 20:17:21 UTC
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

http://rhn.redhat.com/errata/RHBA-2013-0919.html


Note You need to log in before you can comment on or make changes to this bug.