diff --git a/CHANGELOG.md b/CHANGELOG.md index 76c62858..2229a78c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,20 +40,19 @@ Expected: May 2022 ### New features * Extended the Restconf implementation with a limited http-data static service - * Added two new config options to clixon-config.yang: - * `CLICON_HTTP_DATA_PATH` - * `CLICON_HTTP_DATA_ROOT` - * Added feature http-data to restconf-config.yang and the following option: - * `enable-http-data` - * The limited implementation is as follows: - * path: Local static files within `CLICON_WWW_DATA_ROOT` - * operation GET, HEAD, or OPTIONS - * query parameters not supported - * no indata - * media: html, css, js, fonts, image, - 7. Authentication, TLS, http/2 as restconf -Generic changes: - * Uniform path selection across fcgi, native http/1 + http/2 + * The limited implementation is as follows: + * path: Local static files within `CLICON_WWW_DATA_ROOT` + * operation GET, HEAD, or OPTIONS + * query parameters not supported + * no indata + * media: html, css, js, fonts, image. + * authentication, TLS, http/2 as restconf + * Added two new config options to clixon-config.yang: + * `CLICON_HTTP_DATA_PATH` + * `CLICON_HTTP_DATA_ROOT` + * Added feature http-data to restconf-config.yang and the following option that needs to be true + * `enable-http-data` + * Added `HTTP_DATA_INTERNAL_REDIRECT` compile-time option for internal redirects to `index.html` * Implementation of "chunked framing" according to RFC6242 for Netconf 1.1. * First hello is 1.0 EOM framing, then successing rpc is chunked framing @@ -65,10 +64,13 @@ Generic changes: Users may have to change how they access the system +* Restconf + * Added 404 return without body if neither restconf, data or streams prefix match * Netconf: Usage of chunked framing" * To keep existing end-of-message encoding, set `CLICON_NETCONF_BASE_CAPABILITY` to `0` * New `clixon-config@2022-03-21.yang` revision * Added option: + * `CLICON_RESTCONF_API_ROOT` * `CLICON_NETCONF_BASE_CAPABILITY` * `CLICON_HTTP_DATA_PATH` * `CLICON_HTTP_DATA_ROOT` @@ -106,6 +108,7 @@ Users may have to change how they access the system ### Corrected Bugs +* Fixed: HTTP/1 parse error for '/' path * Fixed: YANG if-feature in config file of disables feature did not work, was always on * This does not apply to the datastore, only the config file itself. * Fixed: YANG key list check bad performance diff --git a/apps/restconf/clixon_http1_parse.y b/apps/restconf/clixon_http1_parse.y index 3809ee24..888a0b8e 100644 --- a/apps/restconf/clixon_http1_parse.y +++ b/apps/restconf/clixon_http1_parse.y @@ -41,6 +41,7 @@ %union { char *string; int intval; + void *stack; } %token SP @@ -62,8 +63,8 @@ %token DIGIT %type body -%type absolute_paths -%type absolute_paths1 +%type absolute_paths +%type absolute_paths1 %type absolute_path %type field_vchars %type field_values @@ -259,16 +260,16 @@ method : TOKEN */ request_target : absolute_paths1 { - if (restconf_param_set(_HY->hy_h, "REQUEST_URI", $1) < 0) + if (restconf_param_set(_HY->hy_h, "REQUEST_URI", cbuf_get($1)) < 0) YYABORT; - free($1); + cbuf_free($1); _PARSE_DEBUG("request-target -> absolute-paths1"); } | absolute_paths1 QMARK QUERY { - if (restconf_param_set(_HY->hy_h, "REQUEST_URI", $1) < 0) + if (restconf_param_set(_HY->hy_h, "REQUEST_URI", cbuf_get($1)) < 0) YYABORT; - free($1); + cbuf_free($1); if (http1_parse_query(_HY, $3) < 0) YYABORT; free($3); @@ -285,21 +286,25 @@ absolute_paths1 : absolute_paths { $$ = $1;_PARSE_DEBUG("absolute-paths1 -> absolute-paths / "); } ; -/* absolute-path = 1*( "/" segment ) */ +/* absolute-path = 1*( "/" segment ) +*/ absolute_paths : absolute_paths absolute_path { - if (($$ = clixon_string_del_join($1, "/", $2)) == NULL) { free($2); YYABORT;} - free($2); + $$ = $1; + cprintf($$, "/"); + if ($2) + cprintf($$, "%s", $2); _PARSE_DEBUG("absolute-paths -> absolute-paths absolute-path"); } | absolute_path { - if (($$ = clixon_string_del_join(NULL, "/", $1)) == NULL) { free($1); YYABORT;} - free($1); + if (($$ = cbuf_new()) == NULL){ YYABORT;} + cprintf($$, "/"); + if ($1) + cprintf($$, "%s", $1); _PARSE_DEBUG("absolute-paths -> absolute-path"); } ; - /* segment = * segment = *pchar * pchar = unreserved / pct-encoded / sub-delims / ":" / "@" @@ -310,8 +315,13 @@ absolute_paths : absolute_paths absolute_path */ absolute_path : SLASH PCHARS { - if (($$=strdup($2)) == NULL) YYABORT; - _PARSE_DEBUG("absolute-path -> PCHARS"); + $$ = $2; + _PARSE_DEBUG("absolute-path -> / PCHARS"); + } + | SLASH + { + $$ = NULL; + _PARSE_DEBUG("absolute-path -> /"); } ; diff --git a/apps/restconf/clixon_http_data.c b/apps/restconf/clixon_http_data.c index ba98acb0..adf92ec7 100644 --- a/apps/restconf/clixon_http_data.c +++ b/apps/restconf/clixon_http_data.c @@ -160,69 +160,105 @@ api_http_data_err(clicon_handle h, /*! Check validity of path, may only be regular dir or file * No .., soft link, ~, etc - * @param[in] prefix Prefix of path0, where to start file check - * @param[in] path0 Filepath - * @param[out] code Error code, if retval = 0 - * @retval -1 Error - * @retval 0 Invalid, code set - * @retval 1 OK + * @param[in] h Clicon handle + * @param[in] req Generic Www handle (can be part of clixon handle) + * @param[in] prefix Prefix of path0, where to start file check + * @param[in,out] cbpath Filepath as cbuf, internal redirection may change it + * @param[out] fp Open file, if retval = 1 + * @param[out] fsz Size of file, if retval = 1 + * @retval -1 Error + * @retval 0 Invalid + * @retval 1 OK, fp,fsz set */ static int -check_file_path(char *prefix, - char *path0, - int *code) +http_data_check_file_path(clicon_handle h, + void *req, + char *prefix, + cbuf *cbpath, + FILE **fp, + off_t *fsz) { - int retval = -1; - char *path = NULL; - int i; + int retval = -1; struct stat fstat; + char *p; + int i; + int code = 0; + FILE *f; - if (prefix == NULL || path0 == NULL || code == NULL){ - clicon_err(OE_UNIX, EINVAL, "prefix, path0 or code is NULL"); + if (prefix == NULL || cbpath == NULL || fp == NULL){ + clicon_err(OE_UNIX, EINVAL, "prefix, cbpath0 or fp is NULL"); goto done; } - if ((path = strdup(path0)) == NULL){ - clicon_err(OE_UNIX, errno, "strdup"); + p = cbuf_get(cbpath); + clicon_debug(1, "%s %s", __FUNCTION__, p); + if (strncmp(prefix, p, strlen(prefix)) != 0){ + clicon_err(OE_UNIX, EINVAL, "prefix is not prefix of cbpath"); goto done; } - for (i=strlen(prefix); istrlen(prefix) && path[i-1] == '.'){ - *code = 403; + else if (p[i] == '.' && i>strlen(prefix) && p[i-1] == '.'){ + clicon_debug(1, "%s Error lstat(%s): .. not allowed in file path", __FUNCTION__, p); + code = 403; goto invalid; } } /* Resulting file (ensure not soft link) */ - if (lstat(path, &fstat) < 0){ - *code = 404; + if (lstat(p, &fstat) < 0){ + clicon_debug(1, "%s Error lstat(%s):%s", __FUNCTION__, p, strerror(errno)); + code = 404; goto invalid; } +#ifdef HTTP_DATA_INTERNAL_REDIRECT + /* If dir try redirect, not cbpath is extended */ + if (S_ISDIR(fstat.st_mode)){ + cprintf(cbpath, "/%s", HTTP_DATA_INTERNAL_REDIRECT); + p = cbuf_get(cbpath); + clicon_debug(1, "%s internal redirect: %s", __FUNCTION__, p); + if (lstat(p, &fstat) < 0){ + clicon_debug(1, "%s Error lstat(%s):%s", __FUNCTION__, p, strerror(errno)); + code = 404; + goto invalid; + } + } +#endif if (!S_ISREG(fstat.st_mode)){ - *code = 403; + clicon_debug(1, "%s Error lstat(%s): Not regular file", __FUNCTION__, p); + code = 403; goto invalid; } + *fsz = fstat.st_size; + if ((f = fopen(p, "rb")) == NULL){ + clicon_debug(1, "%s Error fopen(%s) %s", __FUNCTION__, p, strerror(errno)); + code = 403; + goto invalid; + } + *fp = f; retval = 1; /* OK */ done: - if (path) - free(path); return retval; invalid: + if (api_http_data_err(h, req, code) < 0) + goto done; retval = 0; goto done; } @@ -231,10 +267,9 @@ check_file_path(char *prefix, * @param[in] h Clicon handle * @param[in] req Generic Www handle (can be part of clixon handle) * @param[in] pathname With stripped prefix (eg /data), ultimately a filename + * @param[in] head HEAD not GET * @note: primitive file handling, just check if file exists and read it all * XXX 1: Buffer copying once too many, see #if 0 below - * XXX 2: Generic file system below CLICON_HTTP_DATA_ROOT, no checks for links or .. - * Need pathname santitization: no .. or ~, just a directory structure. */ static int api_http_data_file(clicon_handle h, @@ -247,15 +282,16 @@ api_http_data_file(clicon_handle h, char *filename; cbuf *cbdata = NULL; FILE *f = NULL; + off_t fsz = 0; long fsize; char *www_data_root = NULL; char *suffix; char *media; int ret; - int code = 0; char *str = NULL; size_t sz; - + + clicon_debug(1, "%s", __FUNCTION__); if ((cbfile = cbuf_new()) == NULL){ clicon_err(OE_UNIX, errno, "cbuf_new"); goto done; @@ -265,7 +301,24 @@ api_http_data_file(clicon_handle h, goto done; } - if ((suffix = rindex(pathname, '.')) == NULL){ + cprintf(cbfile, "%s", www_data_root); + if (pathname){ + if (strlen(pathname) && pathname[0] != '/'){ + clicon_debug(1, "%s Error fopen(%s) pathname not prefixed with /", + __FUNCTION__, pathname); + if (api_http_data_err(h, req, 404) < 0) + goto done; + goto ok; + } + cprintf(cbfile, "%s", pathname); /* Assume pathname starts with '/' */ + } + if ((ret = http_data_check_file_path(h, req, www_data_root, cbfile, &f, &fsz)) < 0) + goto done; + if (ret == 0) /* Invalid, return code set */ + goto ok; + filename = cbuf_get(cbfile); + /* Find media from file suffix, note there may have been internal indirection */ + if ((suffix = rindex(filename, '.')) == NULL){ media = "application/octet-stream"; } else { @@ -273,29 +326,19 @@ api_http_data_file(clicon_handle h, if ((media = clicon_str2str(mime_map, suffix)) == NULL) media = "application/octet-stream"; } - cprintf(cbfile, "%s", www_data_root); - if (pathname) - cprintf(cbfile, "/%s", pathname); - filename = cbuf_get(cbfile); - clicon_debug(1, "%s %s", __FUNCTION__, filename); - if ((ret = check_file_path(www_data_root, filename, &code)) < 0) - goto done; - if (ret == 0){ - clicon_debug(1, "%s code:%d", __FUNCTION__, code); - if (api_http_data_err(h, req, code) < 0) - goto done; - goto ok; - } - if ((f = fopen(filename, "rb")) == NULL){ - if (api_http_data_err(h, req, 403) < 0) /* Forbidden or 500? */ - goto done; - goto ok; - } /* Size could have been taken from stat() but this reduces the race condition interval * There is still one without flock */ fseek(f, 0, SEEK_END); fsize = ftell(f); + /* Extra sanity check, had some problems with wrong file types */ + if (fsz != fsize){ + clicon_debug(1, "%s Error file %s size mismatch sz:%zu vs %zu", + __FUNCTION__, filename, fsz, fsize); + if (api_http_data_err(h, req, 500) < 0) /* Internal error? */ + goto done; + goto ok; + } fseek(f, 0, SEEK_SET); /* same as rewind(f); */ if ((cbdata = cbuf_new_alloc(fsize+1)) == NULL){ clicon_err(OE_UNIX, errno, "cbuf_new_alloc"); @@ -313,11 +356,11 @@ api_http_data_file(clicon_handle h, goto done; } if (sz != 1){ + clicon_debug(1, "%s Error fread(%s) sz:%zu", __FUNCTION__, filename, sz); if (api_http_data_err(h, req, 500) < 0) /* Internal error? */ goto done; goto ok; } - clicon_debug(1, "%s code:%d", __FUNCTION__, code); str[fsize] = 0; if (cbuf_append_str(cbdata, str) < 0){ clicon_err(OE_UNIX, errno, "cbuf_append_str"); @@ -329,6 +372,7 @@ api_http_data_file(clicon_handle h, goto done; cbdata = NULL; /* consumed by reply-send */ ok: + clicon_debug(1, "%s Read %s OK", __FUNCTION__, filename); retval = 0; done: if (str) diff --git a/apps/restconf/restconf_http1.c b/apps/restconf/restconf_http1.c index 7b3332e5..ae01df35 100644 --- a/apps/restconf/restconf_http1.c +++ b/apps/restconf/restconf_http1.c @@ -438,8 +438,8 @@ restconf_http1_path_root(clicon_handle h, if (api_http_data(h, sd, sd->sd_qvec) < 0) goto done; } - else if (api_root_restconf(h, sd, sd->sd_qvec) < 0) /* error handling */ - goto done; + else + sd->sd_code = 404; /* catch all without body/media */ fail: if (restconf_param_del_all(h) < 0) goto done; diff --git a/apps/restconf/restconf_nghttp2.c b/apps/restconf/restconf_nghttp2.c index 4e1a222c..a67b5acd 100644 --- a/apps/restconf/restconf_nghttp2.c +++ b/apps/restconf/restconf_nghttp2.c @@ -473,9 +473,9 @@ http2_exec(restconf_conn *rc, if ((sd->sd_path = restconf_uripath(rc->rc_h)) == NULL) goto done; sd->sd_proto = HTTP_2; /* XXX is this necessary? */ - if (strncmp(sd->sd_path, "/" RESTCONF_API, strlen("/" RESTCONF_API)) == 0 - || strcmp(sd->sd_path, RESTCONF_WELL_KNOWN) == 0 - || api_path_is_data(rc->rc_h)){ + if (strcmp(sd->sd_path, RESTCONF_WELL_KNOWN) == 0 + || api_path_is_restconf(rc->rc_h) + || api_path_is_data(rc->rc_h)){ if (restconf_nghttp2_path(sd) < 0) goto done; } diff --git a/apps/restconf/restconf_root.c b/apps/restconf/restconf_root.c index e5893dcb..7b25577b 100644 --- a/apps/restconf/restconf_root.c +++ b/apps/restconf/restconf_root.c @@ -80,15 +80,15 @@ api_path_is_restconf(clicon_handle h) { int retval = 0; char *path = NULL; - char *restconf_path = RESTCONF_API; + char *restconf_api_path; if ((path = restconf_uripath(h)) == NULL) goto done; - if (strlen(path) < 1 + strlen(restconf_path)) /* "/" + restconf */ + if ((restconf_api_path = clicon_option_str(h, "CLICON_RESTCONF_API_ROOT")) == NULL) + goto done; + if (strlen(path) < strlen(restconf_api_path)) /* "/" + restconf */ goto done; - if (path[0] != '/') - goto done; - if (strncmp(path+1, restconf_path, strlen(restconf_path)) != 0) + if (strncmp(path, restconf_api_path, strlen(restconf_api_path)) != 0) goto done; retval = 1; done: @@ -510,14 +510,7 @@ api_root_restconf(clicon_handle h, goto ok; } if (strlen(pvec[0]) != 0){ - if (netconf_invalid_value_xml(&xerr, "protocol", "Invalid path, /restconf/ expected") < 0) - goto done; - if (api_return_err0(h, req, xerr, pretty, media_out, 0) < 0) - goto done; - goto ok; - } - if (strcmp(pvec[1], RESTCONF_API)){ - if (netconf_invalid_value_xml(&xerr, "protocol", "Invalid path, /restconf/ expected") < 0) + if (netconf_invalid_value_xml(&xerr, "protocol", "Invalid path, restconf api root expected") < 0) goto done; if (api_return_err0(h, req, xerr, pretty, media_out, 0) < 0) goto done; diff --git a/apps/restconf/restconf_root.h b/apps/restconf/restconf_root.h index 76e9edbe..85e4f1bf 100644 --- a/apps/restconf/restconf_root.h +++ b/apps/restconf/restconf_root.h @@ -41,7 +41,6 @@ /* * Constants */ -#define RESTCONF_API "restconf" #define IETF_YANG_LIBRARY_REVISION "2019-01-04" /* RESTCONF enables deployments to specify where the RESTCONF API is diff --git a/include/clixon_custom.h b/include/clixon_custom.h index 16a3700b..064d8973 100644 --- a/include/clixon_custom.h +++ b/include/clixon_custom.h @@ -99,6 +99,12 @@ */ #define RESTCONF_NETNS_DEFAULT "default" +/*! If set make an internal redirect if URI path indetifies a directory + * For example, path is /local, and redirect is 'index.html, the request + * will be redirected to /local/index.html + */ +#define HTTP_DATA_INTERNAL_REDIRECT "index.html" + /*! Set a temporary parent for use in special case "when" xpath calls * Problem is when changing an existing (candidate) in-memory datastore that yang "when" conditionals * should be changed in clixon_datastore_write.c:text_modify(). diff --git a/test/test_http_data.sh b/test/test_http_data.sh index 44292c80..1e9cec07 100755 --- a/test/test_http_data.sh +++ b/test/test_http_data.sh @@ -4,7 +4,7 @@ # Get them via http and https # Send options and head request # Errors: not found, post, -# XXX: feature disabled +# See RFC 7230 # Magic line must be first in script (see README.md) s="$_" ; . ./lib.sh || if [ "$s" = $0 ]; then exit 0; else return 0; fi @@ -125,12 +125,15 @@ function testrun() RESTCONFIG=$(restconf_config none false $proto $enable) - datapath=/data - wdir=$dir/www -# Host setup: -# datapath=/ -# wdir=/var/www/html - + if true; then + # Proper test setup + datapath=/data + wdir=$dir/www + else + # Experiments with local host + datapath=/ + wdir=/var/www/html + fi # Clixon config cat < $cfg @@ -191,9 +194,24 @@ EOF # echo "curl $CURLOPTS -X GET -H 'Accept: text/html' $proto://localhost/data/index.html" expectpart "$(curl $CURLOPTS -X GET -H 'Accept: text/html' $proto://localhost/data/index.html)" 0 "HTTP/$HVER 404" else - new "WWW get html" + new "WWW get root expect 404 without body" + expectpart "$(curl $CURLOPTS -X GET -H 'Accept: text/html' $proto://localhost/)" 0 "HTTP/$HVER 404" --not-- "Content-Type" + + new "WWW get index.html" expectpart "$(curl $CURLOPTS -X GET -H 'Accept: text/html' $proto://localhost/data/index.html)" 0 "HTTP/$HVER 200" "Content-Type: text/html" "Welcome to Clixon!" + + new "WWW get dir -> expect index.html" + expectpart "$(curl $CURLOPTS -X GET -H 'Accept: text/html' $proto://localhost/data)" 0 "HTTP/$HVER 200" "Content-Type: text/html" "Welcome to Clixon!" + # remove index + mv $dir/www/data/index.html $dir/www/data/tmp.index.html + + new "WWW get dir -> no indirection expect 404" + expectpart "$(curl $CURLOPTS -X GET -H 'Accept: text/html' $proto://localhost/data)" 0 "HTTP/$HVER 404" "Content-Type: text/html" "404 Not Found" + + # move index back + mv $dir/www/data/tmp.index.html $dir/www/data/index.html + new "WWW get css" expectpart "$(curl $CURLOPTS -X GET -H 'Accept: text/html' $proto://localhost/data/example.css)" 0 "HTTP/$HVER 200" "Content-Type: text/css" "display: inline;" --not-- "Content-Type: text/html" @@ -209,6 +227,7 @@ EOF new "WWW get http soft link" expectpart "$(curl $CURLOPTS -X GET -H 'Accept: text/html' $proto://localhost/data/inside.html)" 0 "HTTP/$HVER 403" "Content-Type: text/html" "403 Forbidden" --not-- "Dont access this" + if [ ! -f /.dockerenv ] ; then # XXX Privs dont not work on docker/alpine? new "WWW get http not read access" diff --git a/yang/clixon/clixon-config@2022-03-21.yang b/yang/clixon/clixon-config@2022-03-21.yang index c1e9eb3c..1903de31 100644 --- a/yang/clixon/clixon-config@2022-03-21.yang +++ b/yang/clixon/clixon-config@2022-03-21.yang @@ -49,6 +49,7 @@ module clixon-config { revision 2022-03-21 { description "Added option: + CLICON_RESTCONF_API_ROOT CLICON_NETCONF_BASE_CAPABILITY CLICON_HTTP_DATA_PATH CLICON_HTTP_DATA_ROOT @@ -535,6 +536,13 @@ module clixon-config { RFC6242 for example. This only applies to the external NETCONF"; } + leaf CLICON_RESTCONF_API_ROOT { + type string; + default "/restconf"; + description + "The RESTCONF API root path + See RFC 8040 Sec 1.16 and 3.1"; + } leaf CLICON_RESTCONF_DIR { type string; description @@ -617,7 +625,7 @@ module clixon-config { Both feature clixon-restconf:http-data and restconf/enable-http-data must be enabled for this match to occur."; } - leaf CLICON_HTTP_DATA_ROOT { + leaf CLICON_HTTP_DATA_ROOT{ if-feature "clrc:http-data"; type string; default "/var/www"; @@ -1065,7 +1073,9 @@ module clixon-config { default "streams"; description "Stream path appended to CLICON_STREAM_URL to form - stream subscription URL."; + stream subscription URL. + See CLICON_RESTCONF_API_ROOT and CLICON_HTTP_DATA_ROOT + Should be changed to include '/' "; } leaf CLICON_STREAM_URL { type string;