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
