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.

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-6682: Implement TLS options for PXF external tables #141

Open
wants to merge 5 commits into
base: pxf-6.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
33 changes: 33 additions & 0 deletions external-table/src/libchurl.c
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,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);
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 @@ -415,11 +416,43 @@ 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);

churl_headers_set(context, headers);

return (CHURL_HANDLE) context;
}

static void
set_curl_ssl_options(churl_context *context)
{
const char *proto = get_pxf_protocol();

if (proto && strcmp(proto, "https") == 0 )
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
if (proto && strcmp(proto, "https") == 0 )
if (proto && strcmp(proto, "https") == 0)

Choose a reason for hiding this comment

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

okay

{

const char *keypasswd = get_pxf_ssl_keypasswd();
const char *cacert = get_pxf_ssl_cacert(); // PXF_SSL_CACERT

set_curl_option(context, CURLOPT_SSLCERT, get_pxf_ssl_cert());
set_curl_option(context, CURLOPT_SSLKEY, get_pxf_ssl_key());

set_curl_option(context, CURLOPT_SSLCERTTYPE, get_pxf_ssl_certtype());
if (keypasswd != NULL)
{
set_curl_option(context, CURLOPT_SSLKEYPASSWD, keypasswd);
}

if (cacert != NULL && cacert[0] != '\0')
{
set_curl_option(context, CURLOPT_CAINFO, cacert);
}

set_curl_option(context, CURLOPT_SSL_VERIFYPEER, (const void *)get_pxf_ssl_verifypeer());
}
}

CHURL_HANDLE
churl_init_upload(const char *url, CHURL_HEADERS headers)
{
Expand Down
13 changes: 7 additions & 6 deletions external-table/src/pxfbridge.c
Original file line number Diff line number Diff line change
Expand Up @@ -230,8 +230,8 @@ static void
build_uri_for_cancel(pxfbridge_cancel *cancel)
{
resetStringInfo(&cancel->uri);
appendStringInfo(&cancel->uri, "http://%s/%s/cancel",
get_authority(), PXF_SERVICE_PREFIX);
appendStringInfo(&cancel->uri, "%s://%s/%s/cancel",
get_pxf_protocol(), get_authority(), PXF_SERVICE_PREFIX);

if ((LOG >= log_min_messages) || (LOG >= client_min_messages))
{
Expand All @@ -248,8 +248,8 @@ static void
build_uri_for_read(gphadoop_context *context)
{
resetStringInfo(&context->uri);
appendStringInfo(&context->uri, "http://%s/%s/read",
get_authority(), PXF_SERVICE_PREFIX);
appendStringInfo(&context->uri, "%s://%s/%s/read",
get_pxf_protocol(), get_authority(), PXF_SERVICE_PREFIX);

if ((LOG >= log_min_messages) || (LOG >= client_min_messages))
{
Expand All @@ -265,8 +265,9 @@ build_uri_for_read(gphadoop_context *context)
static void
build_uri_for_write(gphadoop_context *context)
{
appendStringInfo(&context->uri, "http://%s/%s/write",
get_authority(), PXF_SERVICE_PREFIX);

appendStringInfo(&context->uri, "%s://%s/%s/write",
get_pxf_protocol(), get_authority(), PXF_SERVICE_PREFIX);

if ((LOG >= log_min_messages) || (LOG >= client_min_messages))
{
Expand Down
91 changes: 91 additions & 0 deletions external-table/src/pxfutils.c
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,18 @@ get_authority(void)
return psprintf("%s:%d", get_pxf_host(), get_pxf_port());
}

const char *
get_pxf_protocol(void)
{
const char *proto = getenv(ENV_PXF_PROTOCOL);

if (proto == NULL) {
Copy link
Member Author

Choose a reason for hiding this comment

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

An opening bracket after if should be in the next line - this is code style of PostgreSQL. As an alternative the brackets can be removed here

Choose a reason for hiding this comment

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

okay

proto = PXF_DEFAULT_PROTOCOL;
}

return proto;
}

/* Returns the PXF Host defined in the PXF_HOST
* environment variable or the default when undefined
*/
Expand Down Expand Up @@ -116,6 +128,85 @@ get_pxf_port(void)
return port;
}

const char *
get_pxf_ssl_keypasswd(void)
Copy link
Member

@RekGRpth RekGRpth Dec 9, 2024

Choose a reason for hiding this comment

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

how about only two functions

const char *getenv_char(const char *name, const char *default);
long getenv_long(const char *name, long default);

Choose a reason for hiding this comment

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

Nice idea, implemented.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I meant to get rid of the new get_pxf_ssl_* functions completely, and instead just use the two getenv_char and getenv_long functions.

Copy link

@dkovalev1 dkovalev1 Dec 16, 2024

Choose a reason for hiding this comment

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

get_pxf_ssl_* functions keep PXF-related setup isolated in the pxfutils.c, continuing tradition started with get_pxf_port(), get_authority(), etc. It keeps code less coupled.
If fact I'd rename get_pxf_ssl_* functions to get_ssl_* to make churl logically less dependent from PXF, what do you think?

{
const char *proto = getenv(ENV_PXF_SSL_KEYPASSWD);

if (proto == NULL)
{
proto = PXF_DEFAULT_SSL_KEYPASSWD;
}

return proto;
}

const char *
get_pxf_ssl_cacert(void)
{
const char *cacert = getenv(ENV_PXF_SSL_CACERT);

if (cacert == NULL)
{
cacert = PXF_DEFAULT_SSL_CACERT;
}

return cacert;
}

const char *
get_pxf_ssl_cert(void)
{
const char *cert = getenv(ENV_PXF_SSL_CERT);

if (cert == NULL)
{
cert = PXF_DEFAULT_SSL_CERT;
}

return cert;
}

const char *
get_pxf_ssl_key(void)
{
const char *key = getenv(ENV_PXF_SSL_KEY);

if (key == NULL)
{
key = PXF_DEFAULT_SSL_KEY;
}

return key;
}

const char *
get_pxf_ssl_certtype(void)
{
const char *certtype = getenv(ENV_PXF_SSL_CERT_TYPE);

if (certtype == NULL)
{
certtype = PXF_DEFAULT_SSL_CERT_TYPE;
}

return certtype;
}

long
get_pxf_ssl_verifypeer(void)
{
const char *verify_peer_var = getenv(ENV_PXF_SSL_VERIFY_PEER);
long verifypeer = PXF_DEFAULT_SSL_VERIFY_PEER;

if (verify_peer_var != NULL)
{
verifypeer = atol(verify_peer_var);
}

return verifypeer;
Copy link
Member Author

Choose a reason for hiding this comment

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

I suggest to remove the verifypeer variable

Suggested change
long verifypeer = PXF_DEFAULT_SSL_VERIFY_PEER;
if (verify_peer_var != NULL)
{
verifypeer = atol(verify_peer_var);
}
return verifypeer;
if (verify_peer_var != NULL)
{
return atol(verify_peer_var);
}
return PXF_DEFAULT_SSL_VERIFY_PEER;

Choose a reason for hiding this comment

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

This code rewritten now.

}

/* Returns the namespace (schema) name for a given namespace oid */
char *
GetNamespaceName(Oid nsp_oid)
Expand Down
43 changes: 34 additions & 9 deletions external-table/src/pxfutils.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ char *TypeOidGetTypename(Oid typid);
/* Concatenate multiple literal strings using stringinfo */
char *concat(int num_args,...);

/* Get protocol for the PXF server URL */
const char *get_pxf_protocol(void);

/* Get authority (host:port) for the PXF server URL */
char *get_authority(void);

Expand All @@ -25,17 +28,39 @@ const char *get_pxf_host(void);
*/
const int get_pxf_port(void);

const char *get_pxf_ssl_keypasswd(void);
const char *get_pxf_ssl_cacert(void);
const char *get_pxf_ssl_cert(void);
const char *get_pxf_ssl_key(void);
const char *get_pxf_ssl_certtype(void);
long get_pxf_ssl_verifypeer(void);

/* Returns the namespace (schema) name for a given namespace oid */
char *GetNamespaceName(Oid nsp_oid);

#define PXF_PROFILE "PROFILE"
#define FRAGMENTER "FRAGMENTER"
#define ACCESSOR "ACCESSOR"
#define RESOLVER "RESOLVER"
#define ANALYZER "ANALYZER"
#define ENV_PXF_HOST "PXF_HOST"
#define ENV_PXF_PORT "PXF_PORT"
#define PXF_DEFAULT_HOST "localhost"
#define PXF_DEFAULT_PORT 5888
#define PXF_PROFILE "PROFILE"
#define FRAGMENTER "FRAGMENTER"
#define ACCESSOR "ACCESSOR"
#define RESOLVER "RESOLVER"
#define ANALYZER "ANALYZER"
#define ENV_PXF_HOST "PXF_HOST"
#define ENV_PXF_PORT "PXF_PORT"
#define ENV_PXF_PROTOCOL "PXF_PROTOCOL"
#define ENV_PXF_SSL_CACERT "PXF_SSL_CACERT"
#define ENV_PXF_SSL_CERT "PXF_SSL_CERT"
#define ENV_PXF_SSL_CERT_TYPE "PXF_SSL_CERT_TYPE"
#define ENV_PXF_SSL_KEY "PXF_SSL_KEY"
#define ENV_PXF_SSL_KEYPASSWD "PXF_SSL_KEYPASSWD"
#define ENV_PXF_SSL_VERIFY_PEER "PXF_SSL_VERIFY_PEER"
#define PXF_DEFAULT_HOST "localhost"
#define PXF_DEFAULT_PORT 5888
#define PXF_DEFAULT_PROTOCOL "http"
#define PXF_DEFAULT_SSL_CACERT "/home/gpadmin/arenadata_configs/cacert.pem"
#define PXF_DEFAULT_SSL_CERT "client.p12"
#define PXF_DEFAULT_SSL_CERT_TYPE "P12"
#define PXF_DEFAULT_SSL_KEY ""
#define PXF_DEFAULT_SSL_KEYPASSWD ""
#define PXF_DEFAULT_SSL_VERIFY_PEER 1


#endif /* _PXFUTILS_H_ */