Re: [Freeipa-devel] [PATCH] 0102..0105 Better handling for cert-request to disabled CA
On 09/06/2016 04:51 PM, Fraser Tweedale wrote:
On Tue, Aug 30, 2016 at 10:54:32AM +0200, Martin Babinsky wrote:
On 08/26/2016 04:19 AM, Fraser Tweedale wrote:
The attached patches add better handling of cert-request failure due
to target CA being disabled (#6260). To do this, rather than go and
do extra work in Dogtag that we would depend on, instead I bite the
bullet and refactor ra.request_certificate to use the Dogtag REST
API, which correctly responds with status 409 in this case.
Switching RA to Dogtag REST API is an old ticket (#3437) so these
patches address it in part, and show the way forward for the rest of
it.
These patches don't technically depend on patch 0101 which adds the
ca-enable and ca-disable commands, but 0101 may help for testing :)
Thanks,
Fraser
Hi Fraser,
PATCH 102:
LGTM, but please use the standard ":param " annotations in the docstring for
`_ssldo` method. It will make out life easier if we decide to use Sphinx or
similar tool to auto-generate documentation from sources.
You can also add ":raises:" section describing that RemoteRetrieveError is
raised when use_session is True but the session cookie wasn't acquired. It
is kind of obvious but it may trip the uninitiated.
PATCH 103:
Due to magical behavior of our public errors, the exception body should look
like this:
--- a/ipalib/errors.py
+++ b/ipalib/errors.py
@@ -1413,10 +1413,7 @@ class HTTPRequestError(RemoteRetrieveError):
"""
errno = 4035
-
-def __init__(self, status=None, **kw):
-assert status is not None
-super(HTTPRequestError, self).__init__(status=status, **kw)
+format = _('Request failed with status %(status)s: %(reason)')
The format string will be then automatically be supplied with status and
reason if you pass them to the constructor ass you already do. The errors
will be also handled magically (such as status which is None etc.)
PATCH 104:
1.) please don't use bare except here:
"""
+try:
+resp_obj = json.loads(http_body)
+except:
+raise errors.RemoteRetrieveError(reason=_("Response from CA was
not valid JSON"))
"""
use 'except Exception' at least.
PATCH 105:
+if e.status == 409: # pylint: disable=E1101
+raise errors.CertificateOperationError(
+error=_("CA '%s' is disabled") % ca)
+else:
+raise e
+
please use named errors instead of error codes in pylint annotations:
# pylint: disable=no-member
Thanks for your review, Martin. Updated patches attached; they
address all mentioned issues.
Cheers,
Fraser
Thanks, ACK.
Pushed to:
ipa-4-4: b8491490c2dbb3b2db3ce64cd154b499142bc250
master: 520ad7d865ff147d3ff8819d3e384d7cbd69bfb7
--
Martin^3 Babinsky
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH] 0102..0105 Better handling for cert-request to disabled CA
On Tue, Aug 30, 2016 at 10:54:32AM +0200, Martin Babinsky wrote:
> On 08/26/2016 04:19 AM, Fraser Tweedale wrote:
> > The attached patches add better handling of cert-request failure due
> > to target CA being disabled (#6260). To do this, rather than go and
> > do extra work in Dogtag that we would depend on, instead I bite the
> > bullet and refactor ra.request_certificate to use the Dogtag REST
> > API, which correctly responds with status 409 in this case.
> >
> > Switching RA to Dogtag REST API is an old ticket (#3437) so these
> > patches address it in part, and show the way forward for the rest of
> > it.
> >
> > These patches don't technically depend on patch 0101 which adds the
> > ca-enable and ca-disable commands, but 0101 may help for testing :)
> >
> > Thanks,
> > Fraser
> >
> >
> >
>
> Hi Fraser,
>
> PATCH 102:
>
> LGTM, but please use the standard ":param " annotations in the docstring for
> `_ssldo` method. It will make out life easier if we decide to use Sphinx or
> similar tool to auto-generate documentation from sources.
>
> You can also add ":raises:" section describing that RemoteRetrieveError is
> raised when use_session is True but the session cookie wasn't acquired. It
> is kind of obvious but it may trip the uninitiated.
>
> PATCH 103:
>
> Due to magical behavior of our public errors, the exception body should look
> like this:
>
> --- a/ipalib/errors.py
> +++ b/ipalib/errors.py
> @@ -1413,10 +1413,7 @@ class HTTPRequestError(RemoteRetrieveError):
> """
>
> errno = 4035
> -
> -def __init__(self, status=None, **kw):
> -assert status is not None
> -super(HTTPRequestError, self).__init__(status=status, **kw)
> +format = _('Request failed with status %(status)s: %(reason)')
>
> The format string will be then automatically be supplied with status and
> reason if you pass them to the constructor ass you already do. The errors
> will be also handled magically (such as status which is None etc.)
>
> PATCH 104:
>
> 1.) please don't use bare except here:
>
> """
> +try:
> +resp_obj = json.loads(http_body)
> +except:
> +raise errors.RemoteRetrieveError(reason=_("Response from CA was
> not valid JSON"))
> """
>
> use 'except Exception' at least.
>
> PATCH 105:
>
> +if e.status == 409: # pylint: disable=E1101
> +raise errors.CertificateOperationError(
> +error=_("CA '%s' is disabled") % ca)
> +else:
> +raise e
> +
>
> please use named errors instead of error codes in pylint annotations:
> # pylint: disable=no-member
>
Thanks for your review, Martin. Updated patches attached; they
address all mentioned issues.
Cheers,
Fraser
From a1aa93ed13a24c9ac946e47ecd49606ebad8bd9e Mon Sep 17 00:00:00 2001
From: Fraser Tweedale
Date: Fri, 26 Aug 2016 08:59:10 +1000
Subject: [PATCH 102/105] Allow Dogtag RestClient to perform requests without
logging in
Currently the Dogtag RestClient '_ssldo' method requires a session
cookie unconditionally, however, not all REST methods require a
session: some do not require authentication at all, and some will
authenticate the agent on the fly.
To avoid unnecessary login/logout requests via the context manager,
add the 'use_session' keyword argument to '_ssldo'. It defaults to
'True' to preserve existing behaviour (session required) but a
caller can set to 'False' to avoid the requirement.
Part of: https://fedorahosted.org/freeipa/ticket/6260
Part of: https://fedorahosted.org/freeipa/ticket/3473
---
ipaserver/plugins/dogtag.py | 36
1 file changed, 24 insertions(+), 12 deletions(-)
diff --git a/ipaserver/plugins/dogtag.py b/ipaserver/plugins/dogtag.py
index
01e5f1383ee135696a8e968793863ce964025094..f3fb2703f4e1ea688e38cecd02c9acc79213eb40
100644
--- a/ipaserver/plugins/dogtag.py
+++ b/ipaserver/plugins/dogtag.py
@@ -2071,26 +2071,38 @@ class RestClient(Backend):
)
self.cookie = None
-def _ssldo(self, method, path, headers=None, body=None):
+def _ssldo(self, method, path, headers=None, body=None, use_session=True):
"""
-:param url: The URL to post to.
-:param kw: Keyword arguments to encode into POST body.
+Perform an HTTPS request.
+
+:param method: HTTP method to use
+:param path: Path component. This will *extend* the path defined for
+the class (if any).
+:param headers: Additional headers to include in the request.
+:param body: Request body.
+:param use_session: If ``True``, session cookie is added to request
+(client must be logged in).
+
:return: (http_status, http_headers, http_body)
as (integer, dict, str)
-Perform an HTTPS request
-"""
-if self.cookie is None:
-raise errors.RemoteRetrieveError(
-reason=_("REST API is not logged in."))
+
Re: [Freeipa-devel] [PATCH] 0102..0105 Better handling for cert-request to disabled CA
On 08/26/2016 04:19 AM, Fraser Tweedale wrote:
The attached patches add better handling of cert-request failure due
to target CA being disabled (#6260). To do this, rather than go and
do extra work in Dogtag that we would depend on, instead I bite the
bullet and refactor ra.request_certificate to use the Dogtag REST
API, which correctly responds with status 409 in this case.
Switching RA to Dogtag REST API is an old ticket (#3437) so these
patches address it in part, and show the way forward for the rest of
it.
These patches don't technically depend on patch 0101 which adds the
ca-enable and ca-disable commands, but 0101 may help for testing :)
Thanks,
Fraser
Hi Fraser,
PATCH 102:
LGTM, but please use the standard ":param " annotations in the docstring
for `_ssldo` method. It will make out life easier if we decide to use
Sphinx or similar tool to auto-generate documentation from sources.
You can also add ":raises:" section describing that RemoteRetrieveError
is raised when use_session is True but the session cookie wasn't
acquired. It is kind of obvious but it may trip the uninitiated.
PATCH 103:
Due to magical behavior of our public errors, the exception body should
look like this:
--- a/ipalib/errors.py
+++ b/ipalib/errors.py
@@ -1413,10 +1413,7 @@ class HTTPRequestError(RemoteRetrieveError):
"""
errno = 4035
-
-def __init__(self, status=None, **kw):
-assert status is not None
-super(HTTPRequestError, self).__init__(status=status, **kw)
+format = _('Request failed with status %(status)s: %(reason)')
The format string will be then automatically be supplied with status and
reason if you pass them to the constructor ass you already do. The
errors will be also handled magically (such as status which is None etc.)
PATCH 104:
1.) please don't use bare except here:
"""
+try:
+resp_obj = json.loads(http_body)
+except:
+raise errors.RemoteRetrieveError(reason=_("Response from CA
was not valid JSON"))
"""
use 'except Exception' at least.
PATCH 105:
+if e.status == 409: # pylint: disable=E1101
+raise errors.CertificateOperationError(
+error=_("CA '%s' is disabled") % ca)
+else:
+raise e
+
please use named errors instead of error codes in pylint annotations:
# pylint: disable=no-member
--
Martin^3 Babinsky
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
[Freeipa-devel] [PATCH] 0102..0105 Better handling for cert-request to disabled CA
The attached patches add better handling of cert-request failure due
to target CA being disabled (#6260). To do this, rather than go and
do extra work in Dogtag that we would depend on, instead I bite the
bullet and refactor ra.request_certificate to use the Dogtag REST
API, which correctly responds with status 409 in this case.
Switching RA to Dogtag REST API is an old ticket (#3437) so these
patches address it in part, and show the way forward for the rest of
it.
These patches don't technically depend on patch 0101 which adds the
ca-enable and ca-disable commands, but 0101 may help for testing :)
Thanks,
Fraser
From 97501fad9bfe64af076a8c1a65bd732ac265b940 Mon Sep 17 00:00:00 2001
From: Fraser Tweedale
Date: Fri, 26 Aug 2016 08:59:10 +1000
Subject: [PATCH 102/105] Allow Dogtag RestClient to perform requests without
logging in
Currently the Dogtag RestClient '_ssldo' method requires a session
cookie unconditionally, however, not all REST methods require a
session: some do not require authentication at all, and some will
authenticate the agent on the fly.
To avoid unnecessary login/logout requests via the context manager,
add the 'use_session' keyword argument to '_ssldo'. It defaults to
'True' to preserve existing behaviour (session required) but a
caller can set to 'False' to avoid the requirement.
Part of: https://fedorahosted.org/freeipa/ticket/6260
Part of: https://fedorahosted.org/freeipa/ticket/3473
---
ipaserver/plugins/dogtag.py | 38 ++
1 file changed, 26 insertions(+), 12 deletions(-)
diff --git a/ipaserver/plugins/dogtag.py b/ipaserver/plugins/dogtag.py
index
01e5f1383ee135696a8e968793863ce964025094..ac3fa40f80f7c23ab5dceda5e20a33812ff82a21
100644
--- a/ipaserver/plugins/dogtag.py
+++ b/ipaserver/plugins/dogtag.py
@@ -2071,26 +2071,40 @@ class RestClient(Backend):
)
self.cookie = None
-def _ssldo(self, method, path, headers=None, body=None):
+def _ssldo(self, method, path, headers=None, body=None, use_session=True):
"""
-:param url: The URL to post to.
-:param kw: Keyword arguments to encode into POST body.
+Perform an HTTPS request.
+
+``method``
+HTTP method to use
+``path``
+Path component. This will *extend* the path defined for
+the class (if any).
+``headers``
+Additional headers to include in the request.
+``body``
+Request body.
+``use_session``
+If ``True``, session cookie is added to request (client
+must be logged in).
+
:return: (http_status, http_headers, http_body)
as (integer, dict, str)
-Perform an HTTPS request
"""
-if self.cookie is None:
-raise errors.RemoteRetrieveError(
-reason=_("REST API is not logged in."))
-
headers = headers or {}
-headers['Cookie'] = self.cookie
+if use_session:
+if self.cookie is None:
+raise errors.RemoteRetrieveError(
+reason=_("REST API is not logged in."))
+headers['Cookie'] = self.cookie
+
+resource = '/ca/rest'
+if self.path is not None:
+resource = os.path.join(resource, self.path)
if path is not None:
-resource = os.path.join('/ca/rest', self.path, path)
-else:
-resource = os.path.join('/ca/rest', self.path)
+resource = os.path.join(resource, path)
# perform main request
status, resp_headers, resp_body = dogtag.https_request(
--
2.5.5
From 5b8c67c2f293cea32f0a492069bab4ce055ed8fa Mon Sep 17 00:00:00 2001
From: Fraser Tweedale
Date: Fri, 26 Aug 2016 09:04:04 +1000
Subject: [PATCH 103/105] Add HTTPRequestError class
Currently, HTTP requests that respond with status not in the 2xx
range raise RemoteRetrieveError. The exception includes no
information about the response status.
Add the 'HTTPRequestError' class which extends 'RemoteRequestError'
with an attribute for the response status, and update the Dogtag
RestClient to raise the new error.
Part of: https://fedorahosted.org/freeipa/ticket/6260
Part of: https://fedorahosted.org/freeipa/ticket/3473
---
ipalib/errors.py| 13 +
ipaserver/plugins/dogtag.py | 3 ++-
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/ipalib/errors.py b/ipalib/errors.py
index
4cc4455b0abf7d2b1366e1ce6dbb3762bc551cc6..268376f513e7ba9dbfbffaa103002ae4859ecc47
100644
--- a/ipalib/errors.py
+++ b/ipalib/errors.py
@@ -1406,6 +1406,19 @@ class
OperationNotSupportedForPrincipalType(ExecutionError):
'%(operation)s is not supported for %(principal_type)s principals')
+class HTTPRequestError(RemoteRetrieveError):
+"""
+**4035** Raised when an HTTP request fails. Includes the response
+status in the ``status`` attribute.
+"""
+
+errno = 4035
+
+
