On Mon, Mar 23, 2015 at 12:46 PM, Michael Paquier
<michael.paqu...@gmail.com> wrote:
> On Mon, Mar 23, 2015 at 1:48 AM, Воронин Дмитрий
> <carriingfat...@yandex.ru> wrote:
>>
>>> Please, attach new version of my patch to commitfest page.
>>
>> Michael, I found a way to attach patch. sorry to trouble.
>
> Cool. Thanks. I am seeing your patch entry here:
> https://commitfest.postgresql.org/5/192/
> I'll try to take a look at it for the next commit fest, but please do
> not expect immediate feedback things are getting wrapped up for 9.5.
OK, so I have looked at this patch in more details. And here are some comments:
1) As this is an upgrade to sslinfo 1.1, sslinfo--1.0.sql is not necessary.
2) contrib/sslinfo/Makefile needs to be updated with
sslinfo--1.0--1.1.sql and sslinfo--1.1.sql.
3) This return type is not necessary:
+ CREATE TYPE extension AS (
+ name text,
+ value text
+ );
+
+ CREATE OR REPLACE FUNCTION ssl_extension_names() RETURNS SETOF extension
+ AS 'MODULE_PATHNAME', 'ssl_extension_names'
+ LANGUAGE C STRICT;
Instead, I think that we should make ssl_extension_names return a
SETOF record with some OUT parameters. Also, using a tuple descriptor
saved in the user context would bring more readability.
4) sslinfo.control needs to be updated to 1.1.
5) I think that it is better to return an empty result should no
client certificate be present or should ssl be disabled for a given
connection. And the patch failed to do that with SRF_RETURN_DONE.
6) The code is critically lacking comments, and contains many typos.
7) The return status of get_extention() is not necessary. All the code
paths calling it simply ERROR should the status be false. It is better
to move the error message directly in the function and remove the
status code.
8) As proposed, the patch adds 3 new functions:
ssl_extension_is_critical, ssl_extension_value and
ssl_extension_names. But actually I am wondering why
ssl_extension_is_critical and ssl_extension_value are actually useful.
I mean, what counts is the extension information about the existing
client certificate, no? Hence I think that we should remove
ssl_extension_is_critical and ssl_extension_value, and extend
ssl_extension_names with a new boolean flag is_critical to determine
if a extension given is critical or not. Let's rename
ssl_extension_names to ssl_extension_info at the same time.
get_extension is not needed anymore with that as well.
Speaking of which, I have rewritten the patch as attached. This looks
way cleaner than the previous version submitted. Dmitry, does that
look fine for you?
I am switching this patch as "Waiting on Author".
Regards,
--
Michael
From 6ab5ec9ea6705eaf196b07e76b04a78fa0a0f220 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@otacoo.com>
Date: Tue, 7 Jul 2015 23:01:24 +0900
Subject: [PATCH] Add ssl_extension_info in sslinfo
This new function provides information about extension in client
certificates: extension name, extension value and critical status.
---
contrib/sslinfo/Makefile | 3 +-
contrib/sslinfo/sslinfo--1.0--1.1.sql | 13 ++
.../sslinfo/{sslinfo--1.0.sql => sslinfo--1.1.sql} | 12 +-
contrib/sslinfo/sslinfo.c | 167 ++++++++++++++++++++-
contrib/sslinfo/sslinfo.control | 2 +-
doc/src/sgml/sslinfo.sgml | 19 +++
6 files changed, 210 insertions(+), 6 deletions(-)
create mode 100644 contrib/sslinfo/sslinfo--1.0--1.1.sql
rename contrib/sslinfo/{sslinfo--1.0.sql => sslinfo--1.1.sql} (81%)
diff --git a/contrib/sslinfo/Makefile b/contrib/sslinfo/Makefile
index 86cbf05..f6c1472 100644
--- a/contrib/sslinfo/Makefile
+++ b/contrib/sslinfo/Makefile
@@ -4,7 +4,8 @@ MODULE_big = sslinfo
OBJS = sslinfo.o $(WIN32RES)
EXTENSION = sslinfo
-DATA = sslinfo--1.0.sql sslinfo--unpackaged--1.0.sql
+DATA = sslinfo--1.0--1.1.sql sslinfo--1.1.sql \
+ sslinfo--unpackaged--1.0.sql
PGFILEDESC = "sslinfo - information about client SSL certificate"
ifdef USE_PGXS
diff --git a/contrib/sslinfo/sslinfo--1.0--1.1.sql b/contrib/sslinfo/sslinfo--1.0--1.1.sql
new file mode 100644
index 0000000..c98a3ae
--- /dev/null
+++ b/contrib/sslinfo/sslinfo--1.0--1.1.sql
@@ -0,0 +1,13 @@
+/* contrib/sslinfo/sslinfo--1.0--1.1.sql */
+
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "ALTER EXTENSION sslinfo UPDATE TO '1.1'" to load this file. \quit
+
+CREATE OR REPLACE FUNCTION ssl_extension_info(
+ OUT name text,
+ OUT value text,
+ OUT iscritical boolean
+)
+RETURNS SETOF record
+AS 'MODULE_PATHNAME', 'ssl_extension_info'
+LANGUAGE C STRICT;
diff --git a/contrib/sslinfo/sslinfo--1.0.sql b/contrib/sslinfo/sslinfo--1.1.sql
similarity index 81%
rename from contrib/sslinfo/sslinfo--1.0.sql
rename to contrib/sslinfo/sslinfo--1.1.sql
index 79ce656..d63ddd5 100644
--- a/contrib/sslinfo/sslinfo--1.0.sql
+++ b/contrib/sslinfo/sslinfo--1.1.sql
@@ -1,4 +1,4 @@
-/* contrib/sslinfo/sslinfo--1.0.sql */
+/* contrib/sslinfo/sslinfo--1.1.sql */
-- complain if script is sourced in psql, rather than via CREATE EXTENSION
\echo Use "CREATE EXTENSION sslinfo" to load this file. \quit
@@ -38,3 +38,13 @@ LANGUAGE C STRICT;
CREATE FUNCTION ssl_issuer_dn() RETURNS text
AS 'MODULE_PATHNAME', 'ssl_issuer_dn'
LANGUAGE C STRICT;
+
+/* new in 1.1 */
+CREATE OR REPLACE FUNCTION ssl_extension_info(
+ OUT name text,
+ OUT value text,
+ OUT iscritical boolean
+)
+RETURNS SETOF record
+AS 'MODULE_PATHNAME', 'ssl_extension_info'
+LANGUAGE C STRICT;
diff --git a/contrib/sslinfo/sslinfo.c b/contrib/sslinfo/sslinfo.c
index da201bd..07b9ac2 100644
--- a/contrib/sslinfo/sslinfo.c
+++ b/contrib/sslinfo/sslinfo.c
@@ -9,13 +9,18 @@
#include "postgres.h"
#include "fmgr.h"
-#include "utils/numeric.h"
-#include "libpq/libpq-be.h"
+#include "funcapi.h"
#include "miscadmin.h"
-#include "utils/builtins.h"
+
+#include "access/htup_details.h"
+#include "catalog/pg_type.h"
+#include "libpq/libpq-be.h"
#include "mb/pg_wchar.h"
+#include "utils/builtins.h"
+#include "utils/numeric.h"
#include <openssl/x509.h>
+#include <openssl/x509v3.h>
#include <openssl/asn1.h>
PG_MODULE_MAGIC;
@@ -24,6 +29,13 @@ static Datum X509_NAME_field_to_text(X509_NAME *name, text *fieldName);
static Datum X509_NAME_to_text(X509_NAME *name);
static Datum ASN1_STRING_to_text(ASN1_STRING *str);
+/*
+ * Function context for data persisting over repeated calls.
+ */
+typedef struct
+{
+ TupleDesc tupdesc;
+} SSLExtensionInfoContext;
/*
* Indicates whether current session uses SSL
@@ -354,3 +366,152 @@ ssl_issuer_dn(PG_FUNCTION_ARGS)
PG_RETURN_NULL();
return X509_NAME_to_text(X509_get_issuer_name(MyProcPort->peer));
}
+
+
+/*
+ * Returns information about available SSL extensions.
+ *
+ * Returns setof record made of the following values:
+ * - name of the extension.
+ * - value of the extension.
+ * - critical status of the extension.
+ */
+PG_FUNCTION_INFO_V1(ssl_extension_info);
+#define SSLINFO_EXTENSION_INFO_NUM
+Datum
+ssl_extension_info(PG_FUNCTION_ARGS)
+{
+ X509 *cert = MyProcPort->peer;
+ FuncCallContext *funcctx;
+ STACK_OF(X509_EXTENSION) *ext_stack = NULL;
+ int call_cntr;
+ int max_calls;
+ MemoryContext oldcontext;
+ SSLExtensionInfoContext *fctx; /* User function context. */
+
+ if (SRF_IS_FIRSTCALL())
+ {
+
+ TupleDesc tupdesc;
+
+ /* create a function context for cross-call persistence */
+ funcctx = SRF_FIRSTCALL_INIT();
+
+ /*
+ * Switch to memory context appropriate for multiple function calls
+ */
+ oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
+
+ /* Create a user function context for cross-call persistence */
+ fctx = (SSLExtensionInfoContext *) palloc(sizeof(SSLExtensionInfoContext));
+
+ /*
+ * Need a tuple descriptor representing one INT and one TEXT column.
+ */
+ tupdesc = CreateTemplateTupleDesc(3, false);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 1, "name",
+ TEXTOID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 2, "value",
+ TEXTOID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 3, "is_critical",
+ BOOLOID, -1, 0);
+
+ fctx->tupdesc = BlessTupleDesc(tupdesc);
+
+ if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("function returning record called in context "
+ "that cannot accept type record")));
+
+ /* Get all extensions of certificate */
+ if (cert && cert->cert_info)
+ ext_stack = cert->cert_info->extensions;
+
+ /* Set max_calls as a count of extensions in certificate */
+ max_calls = cert != NULL ? X509_get_ext_count(cert) : 0;
+
+ if (cert != NULL &&
+ ext_stack != NULL &&
+ max_calls > 0)
+ {
+ /* got results, keep track of them */
+ funcctx->max_calls = max_calls;
+ funcctx->user_fctx = fctx;
+ }
+ else
+ {
+ /* fast track when no results */
+ MemoryContextSwitchTo(oldcontext);
+ SRF_RETURN_DONE(funcctx);
+ }
+
+ MemoryContextSwitchTo(oldcontext);
+ }
+
+ /* stuff done on every call of the function */
+ funcctx = SRF_PERCALL_SETUP();
+
+ /*
+ * Initialize per-call variables.
+ */
+ call_cntr = funcctx->call_cntr;
+ max_calls = funcctx->max_calls;
+ fctx = funcctx->user_fctx;
+
+ Assert(cert != NULL && cert->cert_info != NULL);
+ ext_stack = cert->cert_info->extensions;
+
+ /* do when there is more left to send */
+ if (call_cntr < max_calls)
+ {
+ Datum values[3];
+ bool nulls[3];
+ char *buf;
+ HeapTuple tuple;
+ Datum result;
+ BIO *membuf = NULL;
+ X509_EXTENSION *ext = NULL;
+ ASN1_OBJECT *obj = NULL;
+ int nid;
+ char nullterm = '\0';
+
+ /* Get extension from certificate */
+ ext = sk_X509_EXTENSION_value(ext_stack, call_cntr);
+ obj = X509_EXTENSION_get_object(ext);
+ nid = OBJ_obj2nid(obj);
+
+ if (nid == NID_undef)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("Unknown extension at position %d",
+ call_cntr)));
+
+ /* Get extension name */
+ values[0] = CStringGetTextDatum(OBJ_nid2sn(nid));
+ nulls[0] = false;
+
+ /* Get next the extension value */
+ membuf = BIO_new(BIO_s_mem());
+ X509V3_EXT_print(membuf, ext, 0, 0);
+ BIO_write(membuf, &nullterm, 1);
+ BIO_get_mem_data(membuf, &buf);
+ values[1] = CStringGetTextDatum(buf);
+ nulls[1] = false;
+
+ /* Get critical status */
+ values[2] = BoolGetDatum(X509_EXTENSION_get_critical(ext));
+ nulls[2] = false;
+
+ /* Build tuple */
+ tuple = heap_form_tuple(fctx->tupdesc, values, nulls);
+ result = HeapTupleGetDatum(tuple);
+
+ BIO_free(membuf);
+
+ SRF_RETURN_NEXT(funcctx, result);
+ }
+
+ /* Do when there is no more left */
+ SRF_RETURN_DONE(funcctx);
+}
diff --git a/contrib/sslinfo/sslinfo.control b/contrib/sslinfo/sslinfo.control
index 1d2f058..dfcf17e 100644
--- a/contrib/sslinfo/sslinfo.control
+++ b/contrib/sslinfo/sslinfo.control
@@ -1,5 +1,5 @@
# sslinfo extension
comment = 'information about SSL certificates'
-default_version = '1.0'
+default_version = '1.1'
module_pathname = '$libdir/sslinfo'
relocatable = true
diff --git a/doc/src/sgml/sslinfo.sgml b/doc/src/sgml/sslinfo.sgml
index 22ef439..bcc27d4 100644
--- a/doc/src/sgml/sslinfo.sgml
+++ b/doc/src/sgml/sslinfo.sgml
@@ -219,6 +219,21 @@ emailAddress
</para>
</listitem>
</varlistentry>
+
+ <varlistentry>
+ <term>
+ <function>ssl_extension_info() returns setof record</function>
+ <indexterm>
+ <primary>ssl_extension_info</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ Provide information about extensions of client certificate: extension name,
+ extension value, and if it is a critical extension.
+ </para>
+ </listitem>
+ </varlistentry>
</variablelist>
</sect2>
@@ -228,6 +243,10 @@ emailAddress
<para>
Victor Wagner <email>vi...@cryptocom.ru</email>, Cryptocom LTD
</para>
+
+ <para>
+ Dmitry Voronin <email>carriingfat...@yandex.ru</email>
+ </para>
<para>
E-Mail of Cryptocom OpenSSL development group:
--
2.4.5
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
