Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ADBDEV-6683 TLS options implemented for PXF FDW #146

Open
wants to merge 19 commits into
base: feature/ADBDEV-6581
Choose a base branch
from
Open

ADBDEV-6683 TLS options implemented for PXF FDW #146

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
675c1dd
Implementation according to SRS
dkovalev1 Dec 4, 2024
0592f3c
Revert unwanted change
dkovalev1 Dec 16, 2024
eadd22e
Revert unwanted change
dkovalev1 Dec 16, 2024
c62d1b5
Merge branch 'pxf-6.x' into ADBDEV-6683
dkovalev1 Dec 16, 2024
3386d35
Implement validation
dkovalev1 Dec 16, 2024
70f5bfb
Merge branch 'pxf-6.x' into ADBDEV-6683
dkovalev1 Dec 16, 2024
e82be59
Rework ssl options initialization for Pxf bridge
dkovalev1 Dec 17, 2024
0075a05
Merge branch 'feature/ADBDEV-6267' into ADBDEV-6683
dkovalev1 Dec 23, 2024
97b5a3a
Adjust indentation
dkovalev1 Dec 23, 2024
74f2f3d
Move SSL options validation to the ValidateOption
dkovalev1 Dec 23, 2024
607a381
Use NULL instead of 0
dkovalev1 Dec 23, 2024
c5d24f7
Implement TLS options for PXF external tables (#141)
andr-sokolov Dec 24, 2024
3f89be8
Whitespace changes
dkovalev1 Jan 9, 2025
5ec9b9d
remove extra tabs
dkovalev1 Jan 10, 2025
695aa30
Merge branch 'feature/ADBDEV-6581' into ADBDEV-6683
dkovalev1 Jan 15, 2025
f0a5d5b
Implement FDW SSL automated test in container
dkovalev1 Jan 15, 2025
d638c69
Add writable test
dkovalev1 Jan 15, 2025
5e13f4c
Merge branch 'feature/ADBDEV-6267' into ADBDEV-6683
dkovalev1 Jan 15, 2025
54981e4
Move SSL related testing to separated container
dkovalev1 Jan 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion external-table/src/libchurl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,7 +448,6 @@ churl_init_upload_timeout(const char *url, CHURL_HEADERS headers, long timeout)
CHURL_HANDLE
churl_init_download(const char *url, CHURL_HEADERS headers)
{
churl_context *context = churl_init(url, headers);
RekGRpth marked this conversation as resolved.
Show resolved Hide resolved

context->upload = false;

Expand Down
45 changes: 38 additions & 7 deletions fdw/libchurl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ static JsonSemAction nullSemAction =
churl_context *churl_new_context(void);
static void create_curl_handle(churl_context *context);
static void set_curl_option(churl_context *context, CURLoption option, const void *data);
static void set_curl_ssl_options(churl_context *context, churl_ssl_options *ssl_options);
static size_t read_callback(void *ptr, size_t size, size_t nmemb, void *userdata);
static void setup_multi_handle(churl_context *context);
static void multi_perform(churl_context *context);
Expand Down Expand Up @@ -388,7 +389,7 @@ log_curl_debug(CURL *handle, curl_infotype type, char *data, size_t size, void *
}

static CHURL_HANDLE
churl_init(const char *url, CHURL_HEADERS headers)
churl_init(const char *url, CHURL_HEADERS headers, churl_ssl_options *ssl_options /* = NULL*/)
{
churl_context *context = churl_new_context();

Expand Down Expand Up @@ -422,21 +423,51 @@ churl_init(const char *url, CHURL_HEADERS headers)
set_curl_option(context, CURLOPT_WRITEDATA, context);
set_curl_option(context, CURLOPT_HEADERFUNCTION, header_callback);
set_curl_option(context, CURLOPT_HEADERDATA, context);

set_curl_ssl_options(context, ssl_options);

churl_headers_set(context, headers);

return (CHURL_HANDLE) context;
}

static void
set_curl_ssl_options(churl_context *context, churl_ssl_options *ssl_options)
{
if (ssl_options != NULL) {
const char *cacert = ssl_options->pxf_ssl_cacert; // PXF_SSL_CACERT

if (ssl_options->pxf_ssl_cert)
set_curl_option(context, CURLOPT_SSLCERT, ssl_options->pxf_ssl_cert);

if (ssl_options->pxf_ssl_key)
set_curl_option(context, CURLOPT_SSLKEY, ssl_options->pxf_ssl_key);

if (ssl_options->pxf_ssl_cert_type)
set_curl_option(context, CURLOPT_SSLCERTTYPE, ssl_options->pxf_ssl_cert_type);

if (ssl_options->pxf_ssl_keypasswd != NULL) {
set_curl_option(context, CURLOPT_SSLKEYPASSWD, ssl_options->pxf_ssl_keypasswd);
}

if (cacert != NULL && cacert[0] != '\0') {
hilltracer marked this conversation as resolved.
Show resolved Hide resolved
set_curl_option(context, CURLOPT_CAINFO, cacert);
}

set_curl_option(context, CURLOPT_SSL_VERIFYPEER, (const void *)ssl_options->pxf_ssl_verify_peer);
}
}

CHURL_HANDLE
churl_init_upload(const char *url, CHURL_HEADERS headers)
churl_init_upload(const char *url, CHURL_HEADERS headers, churl_ssl_options *ssl_options)
{
return churl_init_upload_timeout(url, headers, 0);
return churl_init_upload_timeout(url, headers, ssl_options, 0);
}

CHURL_HANDLE
churl_init_upload_timeout(const char *url, CHURL_HEADERS headers, long timeout)
churl_init_upload_timeout(const char *url, CHURL_HEADERS headers, churl_ssl_options *ssl_options, long timeout)
{
churl_context *context = churl_init(url, headers);
churl_context *context = churl_init(url, headers, ssl_options);

context->upload = true;

Expand All @@ -453,9 +484,9 @@ churl_init_upload_timeout(const char *url, CHURL_HEADERS headers, long timeout)
}

CHURL_HANDLE
churl_init_download(const char *url, CHURL_HEADERS headers)
churl_init_download(const char *url, CHURL_HEADERS headers, churl_ssl_options *ssl_options)
{
churl_context *context = churl_init(url, headers);
churl_context *context = churl_init(url, headers, ssl_options);

context->upload = false;

Expand Down
21 changes: 18 additions & 3 deletions fdw/libchurl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,18 @@
typedef void *CHURL_HEADERS;
typedef void *CHURL_HANDLE;

/*
SSL options
*/
typedef struct {
char *pxf_ssl_cacert;
char *pxf_ssl_cert;
char *pxf_ssl_cert_type;
char *pxf_ssl_key;
char *pxf_ssl_keypasswd;
long pxf_ssl_verify_peer;
} churl_ssl_options;

/*
* PUT example
* -----------
Expand Down Expand Up @@ -104,8 +116,10 @@ void churl_headers_cleanup(CHURL_HEADERS headers);
* Start an upload to url
* returns a handle to churl transfer
*/
CHURL_HANDLE churl_init_upload(const char *url, CHURL_HEADERS headers);
CHURL_HANDLE churl_init_upload_timeout(const char *url, CHURL_HEADERS headers, long timeout);
CHURL_HANDLE churl_init_upload(const char *url, CHURL_HEADERS headers,
hilltracer marked this conversation as resolved.
Show resolved Hide resolved
churl_ssl_options *ssl_options);
CHURL_HANDLE churl_init_upload_timeout(const char *url, CHURL_HEADERS headers,
hilltracer marked this conversation as resolved.
Show resolved Hide resolved
churl_ssl_options *ssl_options, long timeout);

/*
* Returns local port of connected handle or 0
Expand All @@ -116,7 +130,8 @@ int churl_get_local_port(CHURL_HANDLE handle);
* Start a download to url
* returns a handle to churl transfer
*/
CHURL_HANDLE churl_init_download(const char *url, CHURL_HEADERS headers);
CHURL_HANDLE churl_init_download(const char *url, CHURL_HEADERS headers,
hilltracer marked this conversation as resolved.
Show resolved Hide resolved
churl_ssl_options *ssl_options);

/*
* Restart a session to a new URL
Expand Down
85 changes: 79 additions & 6 deletions fdw/pxf_bridge.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,10 @@ typedef struct PxfFdwCancelState
CHURL_HANDLE churl_handle;
ResourceOwner owner;
StringInfoData uri;
char *pxf_protocol;
int pxf_port; /* port number for the PXF Service */
char *pxf_host; /* hostname for the PXF Service */
churl_ssl_options *ssl_options; /* NULL if SSL not configured */
} PxfFdwCancelState;

/* helper function declarations */
Expand All @@ -47,6 +49,50 @@ static size_t FillBuffer(PxfFdwScanState *pxfsstate, char *start, int minlen, in
static size_t FillBuffer(PxfFdwScanState *pxfsstate, char *start, size_t size);
#endif

static churl_ssl_options *churl_make_ssl_options(PxfOptions *options)
{
churl_ssl_options *ssl_options = palloc0(sizeof(churl_ssl_options));

if (options->pxf_ssl_cacert)
ssl_options->pxf_ssl_cacert = pstrdup(options->pxf_ssl_cacert);

if (options->pxf_ssl_cert)
ssl_options->pxf_ssl_cert = pstrdup(options->pxf_ssl_cert);

if (options->pxf_ssl_cert_type)
ssl_options->pxf_ssl_cert_type = pstrdup(options->pxf_ssl_cert_type);

if (options->pxf_ssl_key)
ssl_options->pxf_ssl_key = pstrdup(options->pxf_ssl_key);

if (options->pxf_ssl_keypasswd)
ssl_options->pxf_ssl_keypasswd = pstrdup(options->pxf_ssl_keypasswd);

ssl_options->pxf_ssl_verify_peer = options->pxf_ssl_verify_peer;

return ssl_options;
}

static void free_churl_ssl_options(churl_ssl_options *ssl_options)
{
if (ssl_options->pxf_ssl_cacert)
pfree(ssl_options->pxf_ssl_cacert);

if (ssl_options->pxf_ssl_cert)
pfree(ssl_options->pxf_ssl_cert);

if (ssl_options->pxf_ssl_cert_type)
pfree(ssl_options->pxf_ssl_cert_type);

if (ssl_options->pxf_ssl_key)
pfree(ssl_options->pxf_ssl_key);

if (ssl_options->pxf_ssl_keypasswd)
pfree(ssl_options->pxf_ssl_keypasswd);

pfree(ssl_options);
}

static void
PxfBridgeAbortCallback(ResourceReleasePhase phase,
bool isCommit,
Expand Down Expand Up @@ -84,7 +130,7 @@ PxfBridgeCancel(PxfFdwCancelState *pxfcstate)

initStringInfo(&pxfcstate->uri);
BuildUriForCancel(pxfcstate);
churl_handle = churl_init_upload_timeout(pxfcstate->uri.data, pxfcstate->churl_headers, 1L);
churl_handle = churl_init_upload_timeout(pxfcstate->uri.data, pxfcstate->churl_headers, pxfcstate->ssl_options, 1L);

churl_cleanup(churl_handle, false);
}
Expand Down Expand Up @@ -112,9 +158,16 @@ PxfBridgeCancelCleanup(PxfFdwCancelState *pxfcstate)
if (IsAbortInProgress())
PxfBridgeCancel(pxfcstate);

if (pxfcstate->pxf_protocol)
pfree(pxfcstate->pxf_protocol);

if (pxfcstate->pxf_host)
pfree(pxfcstate->pxf_host);

if (pxfcstate->ssl_options) {
free_churl_ssl_options(pxfcstate->ssl_options);
}

pfree(pxfcstate);
}

Expand Down Expand Up @@ -181,6 +234,7 @@ PxfBridgeImportStart(PxfFdwScanState *pxfsstate)
{
MemoryContext oldcontext;
PxfFdwCancelState *pxfcstate;
churl_ssl_options *ssl_options = NULL;

pxfsstate->churl_headers = churl_headers_init();

Expand All @@ -192,11 +246,23 @@ PxfBridgeImportStart(PxfFdwScanState *pxfsstate)
pxfsstate->retrieved_attrs,
pxfsstate->projectionInfo);

pxfsstate->churl_handle = churl_init_download(pxfsstate->uri.data, pxfsstate->churl_headers);
if (strcmp("https", pxfsstate->options->pxf_protocol) == 0) {
ssl_options = churl_make_ssl_options(pxfsstate->options);
}

pxfsstate->churl_handle = churl_init_download(pxfsstate->uri.data, pxfsstate->churl_headers, ssl_options);
if (ssl_options != NULL) {
free_churl_ssl_options(ssl_options);
}
Comment on lines +249 to +256
Copy link
Member

@RekGRpth RekGRpth Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not simply

Suggested change
if (strcmp("https", pxfsstate->options->pxf_protocol) == 0) {
ssl_options = churl_make_ssl_options(pxfsstate->options);
}
pxfsstate->churl_handle = churl_init_download(pxfsstate->uri.data, pxfsstate->churl_headers, ssl_options);
if (ssl_options != NULL) {
free_churl_ssl_options(ssl_options);
}
pxfsstate->churl_handle = churl_init_download(pxfsstate->uri.data, pxfsstate->churl_headers);
if (strcmp("https", pxfsstate->options->pxf_protocol) == 0) {
churl_set_ssl_options(pxfsstate->churl_handle, pxfsstate->options);
}

and in external table do similar thing

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ssl_options has a type churl_ssl_options which has defined in libchurl, while pxfsstate->options is of type PxfOptions defined in the Pxf. Idea of making churl_ssl_options was to not introduce into libchurl extra dependency of higher level PXF.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The churl_set_ssl_options function could be called differently and placed outside of libchurl.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SSL logically looks very like an connection properties and I decided to set them as a part of libchurl.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not do something similar to pxfsstate->churl_headers with a common type for SSL options?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pxfsstate->churl_headers is a void pointer and does not discover specific type structure. Unlike it, churl_ssl_options is typed. Placing it to the pxfsstate breaks the fact that pxfsstate does not now discover churl-dependent things.
Also, instances of churl_ssl_options are used locally and independently, I don't see any need to put it into pxfsstate.


oldcontext = MemoryContextSwitchTo(CurTransactionContext);
pxfcstate = palloc0(sizeof(PxfFdwCancelState));
pxfcstate->pxf_protocol = pstrdup(pxfsstate->options->pxf_protocol);
pxfcstate->pxf_host = pstrdup(pxfsstate->options->pxf_host);
if (ssl_options != 0) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we compare not with NULL? And why do we need to allocate the second time? Can't we use the first allocation?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to allocate it second time to have it in the transaction's memory context. It's not related to the first time allocation.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not to make the first allocation already in the transaction's memory context?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • These instances are independent logically. Second allocation underscores this fact.
  • This is more change-proof, it's easier to keep them independent in case of further modifications.
  • We won't save much of space and time if reuse memory in other context.

/* make another copy for transaction context */
pxfcstate->ssl_options = churl_make_ssl_options(pxfsstate->options);
}
hilltracer marked this conversation as resolved.
Show resolved Hide resolved
MemoryContextSwitchTo(oldcontext);
pxfsstate->pxfcstate = pxfcstate;
pxfcstate->churl_headers = pxfsstate->churl_headers;
Expand All @@ -223,7 +289,7 @@ PxfBridgeExportStart(PxfFdwModifyState *pxfmstate)
NULL,
NULL,
NULL);
pxfmstate->churl_handle = churl_init_upload(pxfmstate->uri.data, pxfmstate->churl_headers);
pxfmstate->churl_handle = churl_init_upload(pxfmstate->uri.data, pxfmstate->churl_headers, pxfmstate->ssl_options);
}

/*
Expand Down Expand Up @@ -280,8 +346,11 @@ PxfBridgeWrite(PxfFdwModifyState *pxfmstate, char *databuf, int datalen)
static void
BuildUriForCancel(PxfFdwCancelState *pxfcstate)
{
const char *proto = strcmp("https", pxfcstate->pxf_protocol) == 0 ? "https" : "http";

resetStringInfo(&pxfcstate->uri);
appendStringInfo(&pxfcstate->uri, "http://%s:%d/%s/cancel", pxfcstate->pxf_host, pxfcstate->pxf_port, PXF_SERVICE_PREFIX);
appendStringInfo(&pxfcstate->uri, "%s://%s:%d/%s/cancel",
proto, pxfcstate->pxf_host, pxfcstate->pxf_port, PXF_SERVICE_PREFIX);
elog(DEBUG2, "pxf_fdw: uri %s for cancel", pxfcstate->uri.data);
}

Expand All @@ -292,9 +361,11 @@ static void
BuildUriForRead(PxfFdwScanState *pxfsstate)
{
PxfOptions *options = pxfsstate->options;
const char *proto = strcmp("https", options->pxf_protocol) == 0 ? "https" : "http";

resetStringInfo(&pxfsstate->uri);
appendStringInfo(&pxfsstate->uri, "http://%s:%d/%s/read", options->pxf_host, options->pxf_port, PXF_SERVICE_PREFIX);
appendStringInfo(&pxfsstate->uri, "%s://%s:%d/%s/read",
proto, options->pxf_host, options->pxf_port, PXF_SERVICE_PREFIX);
elog(DEBUG2, "pxf_fdw: uri %s for read", pxfsstate->uri.data);
}

Expand All @@ -305,9 +376,11 @@ static void
BuildUriForWrite(PxfFdwModifyState *pxfmstate)
{
PxfOptions *options = pxfmstate->options;
const char *proto = strcmp("https", options->pxf_protocol) == 0 ? "https" : "http";

resetStringInfo(&pxfmstate->uri);
appendStringInfo(&pxfmstate->uri, "http://%s:%d/%s/write", options->pxf_host, options->pxf_port, PXF_SERVICE_PREFIX);
appendStringInfo(&pxfmstate->uri, "%s://%s:%d/%s/write",
proto, options->pxf_host, options->pxf_port, PXF_SERVICE_PREFIX);
elog(DEBUG2, "pxf_fdw: uri %s with file name for write: %s", pxfmstate->uri.data, options->resource);
}

Expand Down
1 change: 1 addition & 0 deletions fdw/pxf_bridge.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ typedef struct PxfFdwModifyState

CHURL_HANDLE churl_handle; /* curl handle */
CHURL_HEADERS churl_headers; /* curl headers */
churl_ssl_options *ssl_options; /* NULL if SSL not used */
StringInfoData uri; /* rest endpoint URI for modify */
Relation relation;
PxfOptions *options; /* FDW options */
Expand Down
18 changes: 18 additions & 0 deletions fdw/pxf_option.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,12 @@
#define FDW_OPTION_PXF_HOST "pxf_host"
#define FDW_OPTION_PXF_PORT "pxf_port"
#define FDW_OPTION_PXF_PROTOCOL "pxf_protocol"
#define FDW_OPTION_SSL_CACERT "pxf_ssl_cacert"
#define FDW_OPTION_SSL_CERT "pxf_ssl_cert"
#define FDW_OPTION_SSL_CERT_TYPE "pxf_ssl_cert_type"
#define FDW_OPTION_SSL_KEY "pxf_ssl_key"
#define FDW_OPTION_SSL_KEYPASSWD "pxf_ssl_keypasswd"
#define FDW_OPTION_SSL_VERIFY_PEER "pxf_ssl_verify_peer"
#define FDW_OPTION_REJECT_LIMIT "reject_limit"
#define FDW_OPTION_REJECT_LIMIT_TYPE "reject_limit_type"
#define FDW_OPTION_RESOURCE "resource"
Expand Down Expand Up @@ -446,6 +452,18 @@ PxfGetOptions(Oid foreigntableid)
opt->pxf_port = atoi(defGetString(def));
else if (strcmp(def->defname, FDW_OPTION_PXF_PROTOCOL) == 0)
opt->pxf_protocol = defGetString(def);
else if (strcmp(def->defname, FDW_OPTION_SSL_CACERT) == 0)
opt->pxf_ssl_cacert = defGetString(def);
else if (strcmp(def->defname, FDW_OPTION_SSL_CERT) == 0)
opt->pxf_ssl_cert = defGetString(def);
else if (strcmp(def->defname, FDW_OPTION_SSL_CERT_TYPE) == 0)
opt->pxf_ssl_cert_type = defGetString(def);
else if (strcmp(def->defname, FDW_OPTION_SSL_KEY) == 0)
opt->pxf_ssl_key = defGetString(def);
else if (strcmp(def->defname, FDW_OPTION_SSL_KEYPASSWD) == 0)
opt->pxf_ssl_keypasswd = defGetString(def);
else if (strcmp(def->defname, FDW_OPTION_SSL_VERIFY_PEER) == 0)
opt->pxf_ssl_verify_peer = atol(defGetString(def));
else if (strcmp(def->defname, FDW_OPTION_PROTOCOL) == 0)
opt->protocol = defGetString(def);
else if (strcmp(def->defname, FDW_OPTION_RESOURCE) == 0)
Expand Down
8 changes: 8 additions & 0 deletions fdw/pxf_option.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,14 @@ typedef struct PxfOptions
char *pxf_protocol; /* protocol for the PXF Service (i.e HTTP or
* HTTPS) */

/* SSL options, used when protocol is HTTPS */
const char *pxf_ssl_cacert;
whitehawk marked this conversation as resolved.
Show resolved Hide resolved
const char *pxf_ssl_cert;
const char *pxf_ssl_cert_type;
const char *pxf_ssl_key;
const char *pxf_ssl_keypasswd;
long pxf_ssl_verify_peer;

/* Server doesn't come from options, it is the actual SERVER name */
char *server; /* the name of the external server */

Expand Down