+++ This bug was initially created as a clone of Bug #886525 +++

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.

--- Additional comment from Alex Lourie on 2013-01-01 10:17:19 EST ---

The bug is actually in SDK:

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

--- Additional comment from Michael Pasternak on 2013-01-02 05:52:02 EST ---

(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.

--- Additional comment from Alex Lourie on 2013-01-09 06:01:10 EST ---

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.

--- Additional comment from Michael Pasternak on 2013-01-09 06:21:25 EST ---

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.

--- Additional comment from Michael Pasternak on 2013-01-09 06:26:05 EST ---

(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 ...

--- Additional comment from Alex Lourie on 2013-01-09 07:52:10 EST ---

OK, got it.

Then the fix must come in the iso-uploader.

--- Additional comment from Alex Lourie on 2013-01-09 10:22:58 EST ---

The patch is uploaded for review: http://gerrit.ovirt.org/#/c/10815/

--- Additional comment from Alex Lourie on 2013-01-16 05:14:51 EST ---

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?

--- Additional comment from RHEL Product and Program Management on 2013-01-16 05:32:07 EST ---

This bug report previously had all acks and release flag approved.
However since at least one of its acks has been changed, the
release flag has been reset to ? by the bugbot (pm-rhel).  The
ack needs to become approved before the release flag can become
approved again.

--- Additional comment from Michael Pasternak on 2013-01-16 06:31:50 EST ---

(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.

--- Additional comment from Alon Bar-Lev on 2013-01-16 11:22:57 EST ---

(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.

--- Additional comment from Alex Lourie on 2013-01-16 11:38:54 EST ---


> 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,
> 
>

--- Additional comment from Michael Pasternak on 2013-01-16 12:23:47 EST ---

(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.

--- Additional comment from Michael Pasternak on 2013-01-16 12:34:25 EST ---

(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.

--- Additional comment from Keith Robertson on 2013-01-16 12:47:34 EST ---

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/

--- Additional comment from Michael Pasternak on 2013-01-16 13:08:50 EST ---

(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.

--- Additional comment from Alon Bar-Lev on 2013-01-16 14:16:39 EST ---

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,

--- Additional comment from Michael Pasternak on 2013-01-16 15:02:46 EST ---

(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,

--- Additional comment from Alon Bar-Lev on 2013-01-16 15:08:55 EST ---

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

--- Additional comment from Michael Pasternak on 2013-01-16 15:13:38 EST ---

(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

--- Additional comment from Alon Bar-Lev on 2013-01-16 15:20:11 EST ---

(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.

--- Additional comment from Michael Pasternak on 2013-01-16 15:35:07 EST ---

(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

--- Additional comment from Alon Bar-Lev on 2013-01-16 15:42:35 EST ---

-> 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.

--- Additional comment from Alon Bar-Lev on 2013-01-16 16:02:08 EST ---

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.

--- Additional comment from Michael Pasternak on 2013-01-16 16:09:42 EST ---

(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.

--- Additional comment from Alon Bar-Lev on 2013-01-16 16:12:01 EST ---

It is as there is nothing wrong with the tool.

--- Additional comment from Michael Pasternak on 2013-01-16 16:17:35 EST ---

(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?

--- Additional comment from Alon Bar-Lev on 2013-01-16 16:19:59 EST ---

Barak, Moran, Itamar,

Please review.

Thanks.

--- Additional comment from Michael Pasternak on 2013-01-17 03:14:32 EST ---

(In reply to comment #28)
> Barak, Moran, Itamar,
> 
> Please review.
> 
> Thanks.

Alon,

i stand on my opinion, you have incorrect interpretation of 'insecure' flag,
it not designed to ignore parameters / or make sdk to work in insecure way,
only meaning of 'insecure' is to make CA certificate requirement to go away,

this is how it's works in other linux tools and i don't see why we should work
differently (especially when the requirement is private iso-uploader use-case described in Comment 24) - it will only confuse users.

therefore please do *not* move this BZ to sdk, as this is by-design behaviour,
and i won't change it.

as i mentioned in Comment 25, if this is how you want iso-uploader to work,
you should not pass cert. when --insecure=True.

--- Additional comment from Keith Robertson on 2013-01-17 11:33:42 EST ---

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

--- Additional comment from Michael Pasternak on 2013-01-17 12:51:38 EST ---

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.

--- Additional comment from Keith Robertson on 2013-02-05 08:30:44 EST ---

(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.

--- Additional comment from Michael Pasternak on 2013-02-05 08:49:18 EST ---

(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

--- Additional comment from Keith Robertson on 2013-02-05 10:38:25 EST ---

(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.

--- Additional comment from Alon Bar-Lev on 2013-02-05 11:01:36 EST ---

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.

--- Additional comment from Michael Pasternak on 2013-02-05 11:53:44 EST ---

(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

--- Additional comment from Michael Pasternak on 2013-02-05 12:31:13 EST ---

(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?

--- Additional comment from Alon Bar-Lev on 2013-02-05 12:49:06 EST ---

(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.

--- Additional comment from Alon Bar-Lev on 2013-02-05 12:56:13 EST ---

(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.

--- Additional comment from Alon Bar-Lev on 2013-02-05 13:01:48 EST ---

(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.

--- Additional comment from Michael Pasternak on 2013-02-05 13:25:19 EST ---

(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.

--- Additional comment from Michael Pasternak on 2013-02-05 13:47:36 EST ---

(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.

--- Additional comment from Barak on 2013-02-11 04:22:55 EST ---

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 ?

--- Additional comment from Alon Bar-Lev on 2013-02-12 00:14:03 EST ---

(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!

--- Additional comment from Barak on 2013-02-25 04:12:28 EST ---

(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!