| Message ID | 20180903162233.28325-2-diego.rondini@kynetics.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | None | expand |
Hi Diego, hi Marta, On 03/09/2018 18:22, Diego Rondini wrote: > Add support to authenticate to hawkBit using a gateway security token. > Gateway security token is a single token shared between all the devices of a > given tenant. It is useful during development as a simple authentication > method. For more informations see: > https://www.eclipse.org/hawkbit/concepts/authentication/ > The gateway token can be passed as a command line parameter with the '-g' or > '--gatewaytoken' options or with the 'gatewaytoken' parameter in the > configuration file. > > Signed-off-by: Diego Rondini <diego.rondini@kynetics.com> > Signed-off-by: Marta Todeschini <marta.todeschini@kynetics.com> > --- > examples/configuration/swupdate.cfg | 3 +++ > suricatta/server_hawkbit.c | 40 +++++++++++++++++++---------- > suricatta/server_hawkbit.h | 2 ++ > 3 files changed, 31 insertions(+), 14 deletions(-) > > diff --git a/examples/configuration/swupdate.cfg b/examples/configuration/swupdate.cfg > index e8f7dd0..42ec91f 100644 > --- a/examples/configuration/swupdate.cfg > +++ b/examples/configuration/swupdate.cfg > @@ -107,6 +107,8 @@ identify : ( > # path of the file containing the certificate for SSL connection > # targettoken : string > # hawkBit target security token > +# gatewaytoken : string > +# hawkBit gateway security token > # proxy : string > # in case the server is reached via a proxy > # ciphers : string in the format used by CURL to set the allowed ciphers suite > @@ -132,6 +134,7 @@ suricatta : > sslkey = "/etc/ssl/sslkey"; > sslcert = "/etc/ssl/sslcert"; > targettoken = "3bc13b476cb3962a0c63a5c92beacfh7"; > + gatewaytoken = "99616d4fae39167deddf21cd90047861"; > */ > }; > > diff --git a/suricatta/server_hawkbit.c b/suricatta/server_hawkbit.c > index eabb9dd..97e571d 100644 > --- a/suricatta/server_hawkbit.c > +++ b/suricatta/server_hawkbit.c > @@ -62,6 +62,7 @@ static struct option long_options[] = { > {"retrywait", required_argument, NULL, 'w'}, > {"proxy", optional_argument, NULL, 'y'}, > {"targettoken", required_argument, NULL, 'k'}, > + {"gatewaytoken", required_argument, NULL, 'g'}, > {NULL, 0, NULL, 0}}; > > static unsigned short mandatory_argument_count = 0; > @@ -1476,7 +1477,8 @@ void suricatta_print_help(void) > "resume a download (default: %ds).\n" > "\t -y, --proxy Use proxy. Either give proxy URL, else " > "{http,all}_proxy env is tried.\n" > - "\t -k, --targettoken Set target token.\n", > + "\t -k, --targettoken Set target token.\n" > + "\t -g, --gatewaytoken Set gateway token.\n", > DEFAULT_POLLING_INTERVAL, DEFAULT_RESUME_TRIES, > DEFAULT_RESUME_DELAY); > } > @@ -1527,12 +1529,11 @@ static int suricatta_settings(void *elem, void __attribute__ ((__unused__)) *da > if (strlen(tmp)) > SETSTRING(channel_data_defaults.proxy, tmp); > GET_FIELD_STRING_RESET(LIBCFG_PARSER, elem, "targettoken", tmp); > - if (strlen(tmp)) { > - char *token_header; > - if (asprintf(&token_header, "Authorization: TargetToken %s", tmp)) > - SETSTRING(channel_data_defaults.header, token_header); > - } > - > + if (strlen(tmp)) > + SETSTRING(server_hawkbit.targettoken, tmp); > + GET_FIELD_STRING_RESET(LIBCFG_PARSER, elem, "gatewaytoken", tmp); > + if (strlen(tmp)) > + SETSTRING(server_hawkbit.gatewaytoken, tmp); > return 0; > > } > @@ -1591,7 +1592,7 @@ server_op_res_t server_start(char *fname, int argc, char *argv[]) > > /* reset to optind=1 to parse suricatta's argument vector */ > optind = 1; > - while ((choice = getopt_long(argc, argv, "t:i:c:u:p:xr:y::w:k:", > + while ((choice = getopt_long(argc, argv, "t:i:c:u:p:xr:y::w:k:g:", > long_options, NULL)) != -1) { > switch (choice) { > case 't': > @@ -1603,12 +1604,11 @@ server_op_res_t server_start(char *fname, int argc, char *argv[]) > mandatory_argument_count |= ID_BIT; > break; > case 'k': > - { > - char *token_header; > - if (asprintf(&token_header, "Authorization: TargetToken %s", optarg)) > - SETSTRING(channel_data_defaults.header, token_header); > - break; > - } > + SETSTRING(server_hawkbit.targettoken, optarg); > + break; > + case 'g': > + SETSTRING(server_hawkbit.gatewaytoken, optarg); > + break; > case 'c': > /* When no persistent update state storage is available, > * use command line switch to instruct what to report. > @@ -1688,6 +1688,18 @@ server_op_res_t server_start(char *fname, int argc, char *argv[]) > if (channel_curl_init() != CHANNEL_OK) > return SERVER_EINIT; > > + /* > + * Set hawkBit token(s) header if any has been set via command line > + * or configuration file > + */ > + char *tokens_header = strdup(""); I have tried to imagine why this is required, I do not get it. Can you explain ? IMHO you are just causing a memory leak because the allocated string with strdup (just '\0') is never freed. The first asprintf will overwrite the pointer. > + if (server_hawkbit.targettoken != NULL) > + asprintf(&tokens_header, "Authorization: TargetToken %s\n", server_hawkbit.targettoken); > + if (server_hawkbit.gatewaytoken != NULL) > + asprintf(&tokens_header, "%sAuthorization: GatewayToken %s\n", tokens_header, server_hawkbit.gatewaytoken); > + if (strlen(tokens_header)) > + SETSTRING(channel_data_defaults.header, tokens_header); > + > /* > * Allocate a channel to communicate with the server > */ > diff --git a/suricatta/server_hawkbit.h b/suricatta/server_hawkbit.h > index f851812..e7cf3dc 100644 > --- a/suricatta/server_hawkbit.h > +++ b/suricatta/server_hawkbit.h > @@ -39,6 +39,8 @@ typedef struct { > char *errors[HAWKBIT_MAX_REPORTED_ERRORS]; > int errorcnt; > const char *update_action; > + char *targettoken; > + char *gatewaytoken; > } server_hawkbit_t; > > extern server_hawkbit_t server_hawkbit; > Best regards, Stefano Babic
Hi Stefano, On Mon, Sep 3, 2018 at 7:20 PM, Stefano Babic <sbabic@denx.de> wrote: > Hi Diego, hi Marta, > > On 03/09/2018 18:22, Diego Rondini wrote: >> Add support to authenticate to hawkBit using a gateway security token. >> Gateway security token is a single token shared between all the devices of a >> given tenant. It is useful during development as a simple authentication >> method. For more informations see: >> https://www.eclipse.org/hawkbit/concepts/authentication/ >> The gateway token can be passed as a command line parameter with the '-g' or >> '--gatewaytoken' options or with the 'gatewaytoken' parameter in the >> configuration file. >> >> Signed-off-by: Diego Rondini <diego.rondini@kynetics.com> >> Signed-off-by: Marta Todeschini <marta.todeschini@kynetics.com> >> --- >> examples/configuration/swupdate.cfg | 3 +++ >> suricatta/server_hawkbit.c | 40 +++++++++++++++++++---------- >> suricatta/server_hawkbit.h | 2 ++ >> 3 files changed, 31 insertions(+), 14 deletions(-) >> >> diff --git a/examples/configuration/swupdate.cfg b/examples/configuration/swupdate.cfg >> index e8f7dd0..42ec91f 100644 >> --- a/examples/configuration/swupdate.cfg >> +++ b/examples/configuration/swupdate.cfg >> @@ -107,6 +107,8 @@ identify : ( >> # path of the file containing the certificate for SSL connection >> # targettoken : string >> # hawkBit target security token >> +# gatewaytoken : string >> +# hawkBit gateway security token >> # proxy : string >> # in case the server is reached via a proxy >> # ciphers : string in the format used by CURL to set the allowed ciphers suite >> @@ -132,6 +134,7 @@ suricatta : >> sslkey = "/etc/ssl/sslkey"; >> sslcert = "/etc/ssl/sslcert"; >> targettoken = "3bc13b476cb3962a0c63a5c92beacfh7"; >> + gatewaytoken = "99616d4fae39167deddf21cd90047861"; >> */ >> }; >> >> diff --git a/suricatta/server_hawkbit.c b/suricatta/server_hawkbit.c >> index eabb9dd..97e571d 100644 >> --- a/suricatta/server_hawkbit.c >> +++ b/suricatta/server_hawkbit.c >> @@ -62,6 +62,7 @@ static struct option long_options[] = { >> {"retrywait", required_argument, NULL, 'w'}, >> {"proxy", optional_argument, NULL, 'y'}, >> {"targettoken", required_argument, NULL, 'k'}, >> + {"gatewaytoken", required_argument, NULL, 'g'}, >> {NULL, 0, NULL, 0}}; >> >> static unsigned short mandatory_argument_count = 0; >> @@ -1476,7 +1477,8 @@ void suricatta_print_help(void) >> "resume a download (default: %ds).\n" >> "\t -y, --proxy Use proxy. Either give proxy URL, else " >> "{http,all}_proxy env is tried.\n" >> - "\t -k, --targettoken Set target token.\n", >> + "\t -k, --targettoken Set target token.\n" >> + "\t -g, --gatewaytoken Set gateway token.\n", >> DEFAULT_POLLING_INTERVAL, DEFAULT_RESUME_TRIES, >> DEFAULT_RESUME_DELAY); >> } >> @@ -1527,12 +1529,11 @@ static int suricatta_settings(void *elem, void __attribute__ ((__unused__)) *da >> if (strlen(tmp)) >> SETSTRING(channel_data_defaults.proxy, tmp); >> GET_FIELD_STRING_RESET(LIBCFG_PARSER, elem, "targettoken", tmp); >> - if (strlen(tmp)) { >> - char *token_header; >> - if (asprintf(&token_header, "Authorization: TargetToken %s", tmp)) >> - SETSTRING(channel_data_defaults.header, token_header); >> - } >> - >> + if (strlen(tmp)) >> + SETSTRING(server_hawkbit.targettoken, tmp); >> + GET_FIELD_STRING_RESET(LIBCFG_PARSER, elem, "gatewaytoken", tmp); >> + if (strlen(tmp)) >> + SETSTRING(server_hawkbit.gatewaytoken, tmp); >> return 0; >> >> } >> @@ -1591,7 +1592,7 @@ server_op_res_t server_start(char *fname, int argc, char *argv[]) >> >> /* reset to optind=1 to parse suricatta's argument vector */ >> optind = 1; >> - while ((choice = getopt_long(argc, argv, "t:i:c:u:p:xr:y::w:k:", >> + while ((choice = getopt_long(argc, argv, "t:i:c:u:p:xr:y::w:k:g:", >> long_options, NULL)) != -1) { >> switch (choice) { >> case 't': >> @@ -1603,12 +1604,11 @@ server_op_res_t server_start(char *fname, int argc, char *argv[]) >> mandatory_argument_count |= ID_BIT; >> break; >> case 'k': >> - { >> - char *token_header; >> - if (asprintf(&token_header, "Authorization: TargetToken %s", optarg)) >> - SETSTRING(channel_data_defaults.header, token_header); >> - break; >> - } >> + SETSTRING(server_hawkbit.targettoken, optarg); >> + break; >> + case 'g': >> + SETSTRING(server_hawkbit.gatewaytoken, optarg); >> + break; >> case 'c': >> /* When no persistent update state storage is available, >> * use command line switch to instruct what to report. >> @@ -1688,6 +1688,18 @@ server_op_res_t server_start(char *fname, int argc, char *argv[]) >> if (channel_curl_init() != CHANNEL_OK) >> return SERVER_EINIT; >> >> + /* >> + * Set hawkBit token(s) header if any has been set via command line >> + * or configuration file >> + */ >> + char *tokens_header = strdup(""); > > I have tried to imagine why this is required, I do not get it. Can you > explain ? IMHO you are just causing a memory leak because the allocated > string with strdup (just '\0') is never freed. The first asprintf will > overwrite the pointer. I'm sorry if this was not explained, and thanks for pointing out the memleak. The problem we wanted to solve with that code is the case where both target and gateway token are passed by the user. Possible solutions are: 1) put both Auth methods in the HTTP header 2) spit an error and fail 3) spit a warning and put just one of the two (which one??) Auth methods in the HTTP header We've decided to go with option 1) as the hawkBit server could decide which one (of the two Auth methods provided by the client) to use based on the server Authentication Configuration (enabled Auth methods). In the case both target and gateway tokens are passed we need to concatenate the two strings (which include a "\n"). As you can see, the gateway token's asprintf prepends token_header which at that point of the execution can be either "" or the target token auth string. > >> + if (server_hawkbit.targettoken != NULL) >> + asprintf(&tokens_header, "Authorization: TargetToken %s\n", server_hawkbit.targettoken); >> + if (server_hawkbit.gatewaytoken != NULL) >> + asprintf(&tokens_header, "%sAuthorization: GatewayToken %s\n", tokens_header, server_hawkbit.gatewaytoken); >> + if (strlen(tokens_header)) >> + SETSTRING(channel_data_defaults.header, tokens_header); >> + This is a reworked version with the memleak fixed: char *tokens_header = NULL; if (server_hawkbit.targettoken != NULL) asprintf(&tokens_header, "Authorization: TargetToken %s\n", server_hawkbit.targettoken); if (server_hawkbit.gatewaytoken != NULL) { if (tokens_header == NULL) asprintf(&tokens_header, "Authorization: GatewayToken %s\n", server_hawkbit.gatewaytoken); else asprintf(&tokens_header, "%sAuthorization: GatewayToken %s\n", tokens_header, server_hawkbit.gatewaytoken); } if (tokens_header != NULL && strlen(tokens_header)) SETSTRING(channel_data_defaults.header, tokens_header); Let me know if this one is ok and I'll send V2. >> /* >> * Allocate a channel to communicate with the server >> */ >> diff --git a/suricatta/server_hawkbit.h b/suricatta/server_hawkbit.h >> index f851812..e7cf3dc 100644 >> --- a/suricatta/server_hawkbit.h >> +++ b/suricatta/server_hawkbit.h >> @@ -39,6 +39,8 @@ typedef struct { >> char *errors[HAWKBIT_MAX_REPORTED_ERRORS]; >> int errorcnt; >> const char *update_action; >> + char *targettoken; >> + char *gatewaytoken; >> } server_hawkbit_t; >> >> extern server_hawkbit_t server_hawkbit; >> > > Best regards, > Stefano Babic > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de > ===================================================================== Best regards, Diego Rondini Sr. Embedded Engineer Kynetics www.kynetics.com
Hi Diego, On 04/09/2018 18:03, Diego Rondini wrote: > Hi Stefano, > > On Mon, Sep 3, 2018 at 7:20 PM, Stefano Babic <sbabic@denx.de> wrote: >> Hi Diego, hi Marta, >> >> On 03/09/2018 18:22, Diego Rondini wrote: >>> Add support to authenticate to hawkBit using a gateway security token. >>> Gateway security token is a single token shared between all the devices of a >>> given tenant. It is useful during development as a simple authentication >>> method. For more informations see: >>> https://www.eclipse.org/hawkbit/concepts/authentication/ >>> The gateway token can be passed as a command line parameter with the '-g' or >>> '--gatewaytoken' options or with the 'gatewaytoken' parameter in the >>> configuration file. >>> >>> Signed-off-by: Diego Rondini <diego.rondini@kynetics.com> >>> Signed-off-by: Marta Todeschini <marta.todeschini@kynetics.com> >>> --- >>> examples/configuration/swupdate.cfg | 3 +++ >>> suricatta/server_hawkbit.c | 40 +++++++++++++++++++---------- >>> suricatta/server_hawkbit.h | 2 ++ >>> 3 files changed, 31 insertions(+), 14 deletions(-) >>> >>> diff --git a/examples/configuration/swupdate.cfg b/examples/configuration/swupdate.cfg >>> index e8f7dd0..42ec91f 100644 >>> --- a/examples/configuration/swupdate.cfg >>> +++ b/examples/configuration/swupdate.cfg >>> @@ -107,6 +107,8 @@ identify : ( >>> # path of the file containing the certificate for SSL connection >>> # targettoken : string >>> # hawkBit target security token >>> +# gatewaytoken : string >>> +# hawkBit gateway security token >>> # proxy : string >>> # in case the server is reached via a proxy >>> # ciphers : string in the format used by CURL to set the allowed ciphers suite >>> @@ -132,6 +134,7 @@ suricatta : >>> sslkey = "/etc/ssl/sslkey"; >>> sslcert = "/etc/ssl/sslcert"; >>> targettoken = "3bc13b476cb3962a0c63a5c92beacfh7"; >>> + gatewaytoken = "99616d4fae39167deddf21cd90047861"; >>> */ >>> }; >>> >>> diff --git a/suricatta/server_hawkbit.c b/suricatta/server_hawkbit.c >>> index eabb9dd..97e571d 100644 >>> --- a/suricatta/server_hawkbit.c >>> +++ b/suricatta/server_hawkbit.c >>> @@ -62,6 +62,7 @@ static struct option long_options[] = { >>> {"retrywait", required_argument, NULL, 'w'}, >>> {"proxy", optional_argument, NULL, 'y'}, >>> {"targettoken", required_argument, NULL, 'k'}, >>> + {"gatewaytoken", required_argument, NULL, 'g'}, >>> {NULL, 0, NULL, 0}}; >>> >>> static unsigned short mandatory_argument_count = 0; >>> @@ -1476,7 +1477,8 @@ void suricatta_print_help(void) >>> "resume a download (default: %ds).\n" >>> "\t -y, --proxy Use proxy. Either give proxy URL, else " >>> "{http,all}_proxy env is tried.\n" >>> - "\t -k, --targettoken Set target token.\n", >>> + "\t -k, --targettoken Set target token.\n" >>> + "\t -g, --gatewaytoken Set gateway token.\n", >>> DEFAULT_POLLING_INTERVAL, DEFAULT_RESUME_TRIES, >>> DEFAULT_RESUME_DELAY); >>> } >>> @@ -1527,12 +1529,11 @@ static int suricatta_settings(void *elem, void __attribute__ ((__unused__)) *da >>> if (strlen(tmp)) >>> SETSTRING(channel_data_defaults.proxy, tmp); >>> GET_FIELD_STRING_RESET(LIBCFG_PARSER, elem, "targettoken", tmp); >>> - if (strlen(tmp)) { >>> - char *token_header; >>> - if (asprintf(&token_header, "Authorization: TargetToken %s", tmp)) >>> - SETSTRING(channel_data_defaults.header, token_header); >>> - } >>> - >>> + if (strlen(tmp)) >>> + SETSTRING(server_hawkbit.targettoken, tmp); >>> + GET_FIELD_STRING_RESET(LIBCFG_PARSER, elem, "gatewaytoken", tmp); >>> + if (strlen(tmp)) >>> + SETSTRING(server_hawkbit.gatewaytoken, tmp); >>> return 0; >>> >>> } >>> @@ -1591,7 +1592,7 @@ server_op_res_t server_start(char *fname, int argc, char *argv[]) >>> >>> /* reset to optind=1 to parse suricatta's argument vector */ >>> optind = 1; >>> - while ((choice = getopt_long(argc, argv, "t:i:c:u:p:xr:y::w:k:", >>> + while ((choice = getopt_long(argc, argv, "t:i:c:u:p:xr:y::w:k:g:", >>> long_options, NULL)) != -1) { >>> switch (choice) { >>> case 't': >>> @@ -1603,12 +1604,11 @@ server_op_res_t server_start(char *fname, int argc, char *argv[]) >>> mandatory_argument_count |= ID_BIT; >>> break; >>> case 'k': >>> - { >>> - char *token_header; >>> - if (asprintf(&token_header, "Authorization: TargetToken %s", optarg)) >>> - SETSTRING(channel_data_defaults.header, token_header); >>> - break; >>> - } >>> + SETSTRING(server_hawkbit.targettoken, optarg); >>> + break; >>> + case 'g': >>> + SETSTRING(server_hawkbit.gatewaytoken, optarg); >>> + break; >>> case 'c': >>> /* When no persistent update state storage is available, >>> * use command line switch to instruct what to report. >>> @@ -1688,6 +1688,18 @@ server_op_res_t server_start(char *fname, int argc, char *argv[]) >>> if (channel_curl_init() != CHANNEL_OK) >>> return SERVER_EINIT; >>> >>> + /* >>> + * Set hawkBit token(s) header if any has been set via command line >>> + * or configuration file >>> + */ >>> + char *tokens_header = strdup(""); >> >> I have tried to imagine why this is required, I do not get it. Can you >> explain ? IMHO you are just causing a memory leak because the allocated >> string with strdup (just '\0') is never freed. The first asprintf will >> overwrite the pointer. > > I'm sorry if this was not explained, and thanks for pointing out the memleak. > > The problem we wanted to solve with that code is the case where both > target and gateway token are passed by the user. > Possible solutions are: > 1) put both Auth methods in the HTTP header > 2) spit an error and fail > 3) spit a warning and put just one of the two (which one??) Auth > methods in the HTTP header > > We've decided to go with option 1) as the hawkBit server could decide > which one (of the two Auth methods provided by the client) to use > based on the server Authentication Configuration (enabled Auth > methods). What is Hawkbit's behavior if we put both token into the header ? Really I do not know. And what is the behavior if one token is correct and the second one not ? Is itauthenticated or not ? If it is correct that a device can send both tokens, 1) is correct. But it can be a nightmare if Hawkbit decides to not authenticate a device if one of the token is wrong. If both tokens are not allowed, we should raise an error and 2) is correct. 3) does not make sense IMHO. > In the case both target and gateway tokens are passed we need to > concatenate the two strings (which include a "\n"). > As you can see, the gateway token's asprintf prepends token_header > which at that point of the execution can be either "" or the target > token auth string. > >> >>> + if (server_hawkbit.targettoken != NULL) >>> + asprintf(&tokens_header, "Authorization: TargetToken %s\n", server_hawkbit.targettoken); >>> + if (server_hawkbit.gatewaytoken != NULL) >>> + asprintf(&tokens_header, "%sAuthorization: GatewayToken %s\n", tokens_header, server_hawkbit.gatewaytoken); >>> + if (strlen(tokens_header)) >>> + SETSTRING(channel_data_defaults.header, tokens_header); >>> + > > This is a reworked version with the memleak fixed: > > char *tokens_header = NULL; > if (server_hawkbit.targettoken != NULL) > asprintf(&tokens_header, "Authorization: TargetToken %s\n", > server_hawkbit.targettoken); > if (server_hawkbit.gatewaytoken != NULL) { > if (tokens_header == NULL) > asprintf(&tokens_header, "Authorization: GatewayToken > %s\n", server_hawkbit.gatewaytoken); > else > asprintf(&tokens_header, "%sAuthorization: GatewayToken > %s\n", tokens_header, server_hawkbit.gatewaytoken); > } > if (tokens_header != NULL && strlen(tokens_header)) > SETSTRING(channel_data_defaults.header, tokens_header); > > Let me know if this one is ok and I'll send V2. It would be nice to clarify how Hawkbit behaves. Best regards, Stefano
Hi Stefano, On Tue, Sep 4, 2018 at 6:28 PM, Stefano Babic <sbabic@denx.de> wrote: > Hi Diego, > > On 04/09/2018 18:03, Diego Rondini wrote: >> Hi Stefano, >> >> On Mon, Sep 3, 2018 at 7:20 PM, Stefano Babic <sbabic@denx.de> wrote: >>> Hi Diego, hi Marta, >>> >>> On 03/09/2018 18:22, Diego Rondini wrote: >>>> Add support to authenticate to hawkBit using a gateway security token. >>>> Gateway security token is a single token shared between all the devices of a >>>> given tenant. It is useful during development as a simple authentication >>>> method. For more informations see: >>>> https://www.eclipse.org/hawkbit/concepts/authentication/ >>>> The gateway token can be passed as a command line parameter with the '-g' or >>>> '--gatewaytoken' options or with the 'gatewaytoken' parameter in the >>>> configuration file. >>>> >>>> Signed-off-by: Diego Rondini <diego.rondini@kynetics.com> >>>> Signed-off-by: Marta Todeschini <marta.todeschini@kynetics.com> >>>> --- >>>> examples/configuration/swupdate.cfg | 3 +++ >>>> suricatta/server_hawkbit.c | 40 +++++++++++++++++++---------- >>>> suricatta/server_hawkbit.h | 2 ++ >>>> 3 files changed, 31 insertions(+), 14 deletions(-) >>>> >>>> diff --git a/examples/configuration/swupdate.cfg b/examples/configuration/swupdate.cfg >>>> index e8f7dd0..42ec91f 100644 >>>> --- a/examples/configuration/swupdate.cfg >>>> +++ b/examples/configuration/swupdate.cfg >>>> @@ -107,6 +107,8 @@ identify : ( >>>> # path of the file containing the certificate for SSL connection >>>> # targettoken : string >>>> # hawkBit target security token >>>> +# gatewaytoken : string >>>> +# hawkBit gateway security token >>>> # proxy : string >>>> # in case the server is reached via a proxy >>>> # ciphers : string in the format used by CURL to set the allowed ciphers suite >>>> @@ -132,6 +134,7 @@ suricatta : >>>> sslkey = "/etc/ssl/sslkey"; >>>> sslcert = "/etc/ssl/sslcert"; >>>> targettoken = "3bc13b476cb3962a0c63a5c92beacfh7"; >>>> + gatewaytoken = "99616d4fae39167deddf21cd90047861"; >>>> */ >>>> }; >>>> >>>> diff --git a/suricatta/server_hawkbit.c b/suricatta/server_hawkbit.c >>>> index eabb9dd..97e571d 100644 >>>> --- a/suricatta/server_hawkbit.c >>>> +++ b/suricatta/server_hawkbit.c >>>> @@ -62,6 +62,7 @@ static struct option long_options[] = { >>>> {"retrywait", required_argument, NULL, 'w'}, >>>> {"proxy", optional_argument, NULL, 'y'}, >>>> {"targettoken", required_argument, NULL, 'k'}, >>>> + {"gatewaytoken", required_argument, NULL, 'g'}, >>>> {NULL, 0, NULL, 0}}; >>>> >>>> static unsigned short mandatory_argument_count = 0; >>>> @@ -1476,7 +1477,8 @@ void suricatta_print_help(void) >>>> "resume a download (default: %ds).\n" >>>> "\t -y, --proxy Use proxy. Either give proxy URL, else " >>>> "{http,all}_proxy env is tried.\n" >>>> - "\t -k, --targettoken Set target token.\n", >>>> + "\t -k, --targettoken Set target token.\n" >>>> + "\t -g, --gatewaytoken Set gateway token.\n", >>>> DEFAULT_POLLING_INTERVAL, DEFAULT_RESUME_TRIES, >>>> DEFAULT_RESUME_DELAY); >>>> } >>>> @@ -1527,12 +1529,11 @@ static int suricatta_settings(void *elem, void __attribute__ ((__unused__)) *da >>>> if (strlen(tmp)) >>>> SETSTRING(channel_data_defaults.proxy, tmp); >>>> GET_FIELD_STRING_RESET(LIBCFG_PARSER, elem, "targettoken", tmp); >>>> - if (strlen(tmp)) { >>>> - char *token_header; >>>> - if (asprintf(&token_header, "Authorization: TargetToken %s", tmp)) >>>> - SETSTRING(channel_data_defaults.header, token_header); >>>> - } >>>> - >>>> + if (strlen(tmp)) >>>> + SETSTRING(server_hawkbit.targettoken, tmp); >>>> + GET_FIELD_STRING_RESET(LIBCFG_PARSER, elem, "gatewaytoken", tmp); >>>> + if (strlen(tmp)) >>>> + SETSTRING(server_hawkbit.gatewaytoken, tmp); >>>> return 0; >>>> >>>> } >>>> @@ -1591,7 +1592,7 @@ server_op_res_t server_start(char *fname, int argc, char *argv[]) >>>> >>>> /* reset to optind=1 to parse suricatta's argument vector */ >>>> optind = 1; >>>> - while ((choice = getopt_long(argc, argv, "t:i:c:u:p:xr:y::w:k:", >>>> + while ((choice = getopt_long(argc, argv, "t:i:c:u:p:xr:y::w:k:g:", >>>> long_options, NULL)) != -1) { >>>> switch (choice) { >>>> case 't': >>>> @@ -1603,12 +1604,11 @@ server_op_res_t server_start(char *fname, int argc, char *argv[]) >>>> mandatory_argument_count |= ID_BIT; >>>> break; >>>> case 'k': >>>> - { >>>> - char *token_header; >>>> - if (asprintf(&token_header, "Authorization: TargetToken %s", optarg)) >>>> - SETSTRING(channel_data_defaults.header, token_header); >>>> - break; >>>> - } >>>> + SETSTRING(server_hawkbit.targettoken, optarg); >>>> + break; >>>> + case 'g': >>>> + SETSTRING(server_hawkbit.gatewaytoken, optarg); >>>> + break; >>>> case 'c': >>>> /* When no persistent update state storage is available, >>>> * use command line switch to instruct what to report. >>>> @@ -1688,6 +1688,18 @@ server_op_res_t server_start(char *fname, int argc, char *argv[]) >>>> if (channel_curl_init() != CHANNEL_OK) >>>> return SERVER_EINIT; >>>> >>>> + /* >>>> + * Set hawkBit token(s) header if any has been set via command line >>>> + * or configuration file >>>> + */ >>>> + char *tokens_header = strdup(""); >>> >>> I have tried to imagine why this is required, I do not get it. Can you >>> explain ? IMHO you are just causing a memory leak because the allocated >>> string with strdup (just '\0') is never freed. The first asprintf will >>> overwrite the pointer. >> >> I'm sorry if this was not explained, and thanks for pointing out the memleak. >> >> The problem we wanted to solve with that code is the case where both >> target and gateway token are passed by the user. >> Possible solutions are: >> 1) put both Auth methods in the HTTP header >> 2) spit an error and fail >> 3) spit a warning and put just one of the two (which one??) Auth >> methods in the HTTP header >> >> We've decided to go with option 1) as the hawkBit server could decide >> which one (of the two Auth methods provided by the client) to use >> based on the server Authentication Configuration (enabled Auth >> methods). > > What is Hawkbit's behavior if we put both token into the header ? Really > I do not know. > > And what is the behavior if one token is correct and the second one not > ? Is itauthenticated or not ? > > If it is correct that a device can send both tokens, 1) is correct. But > it can be a nightmare if Hawkbit decides to not authenticate a device if > one of the token is wrong. > > If both tokens are not allowed, we should raise an error and 2) is correct. I just discussed on gitter with the hawkBit maintainers and they will stick with the current hawkBit behaviour of considering just the first match of the field name (Authentication) in the header provided. I will send V2 shortly. > > 3) does not make sense IMHO. > >> In the case both target and gateway tokens are passed we need to >> concatenate the two strings (which include a "\n"). >> As you can see, the gateway token's asprintf prepends token_header >> which at that point of the execution can be either "" or the target >> token auth string. >> >>> >>>> + if (server_hawkbit.targettoken != NULL) >>>> + asprintf(&tokens_header, "Authorization: TargetToken %s\n", server_hawkbit.targettoken); >>>> + if (server_hawkbit.gatewaytoken != NULL) >>>> + asprintf(&tokens_header, "%sAuthorization: GatewayToken %s\n", tokens_header, server_hawkbit.gatewaytoken); >>>> + if (strlen(tokens_header)) >>>> + SETSTRING(channel_data_defaults.header, tokens_header); >>>> + >> >> This is a reworked version with the memleak fixed: >> >> char *tokens_header = NULL; >> if (server_hawkbit.targettoken != NULL) >> asprintf(&tokens_header, "Authorization: TargetToken %s\n", >> server_hawkbit.targettoken); >> if (server_hawkbit.gatewaytoken != NULL) { >> if (tokens_header == NULL) >> asprintf(&tokens_header, "Authorization: GatewayToken >> %s\n", server_hawkbit.gatewaytoken); >> else >> asprintf(&tokens_header, "%sAuthorization: GatewayToken >> %s\n", tokens_header, server_hawkbit.gatewaytoken); >> } >> if (tokens_header != NULL && strlen(tokens_header)) >> SETSTRING(channel_data_defaults.header, tokens_header); >> >> Let me know if this one is ok and I'll send V2. > > It would be nice to clarify how Hawkbit behaves. > > Best regards, > Stefano > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de > ===================================================================== Best regards, Diego Rondini Sr. Embedded Engineer Kynetics www.kynetics.com
Hi Diego, On 05/09/2018 12:08, Diego Rondini wrote: > Hi Stefano, > > On Tue, Sep 4, 2018 at 6:28 PM, Stefano Babic <sbabic@denx.de> wrote: >> Hi Diego, >> >> On 04/09/2018 18:03, Diego Rondini wrote: >>> Hi Stefano, >>> >>> On Mon, Sep 3, 2018 at 7:20 PM, Stefano Babic <sbabic@denx.de> wrote: >>>> Hi Diego, hi Marta, >>>> >>>> On 03/09/2018 18:22, Diego Rondini wrote: >>>>> Add support to authenticate to hawkBit using a gateway security token. >>>>> Gateway security token is a single token shared between all the devices of a >>>>> given tenant. It is useful during development as a simple authentication >>>>> method. For more informations see: >>>>> https://www.eclipse.org/hawkbit/concepts/authentication/ >>>>> The gateway token can be passed as a command line parameter with the '-g' or >>>>> '--gatewaytoken' options or with the 'gatewaytoken' parameter in the >>>>> configuration file. >>>>> >>>>> Signed-off-by: Diego Rondini <diego.rondini@kynetics.com> >>>>> Signed-off-by: Marta Todeschini <marta.todeschini@kynetics.com> >>>>> --- >>>>> examples/configuration/swupdate.cfg | 3 +++ >>>>> suricatta/server_hawkbit.c | 40 +++++++++++++++++++---------- >>>>> suricatta/server_hawkbit.h | 2 ++ >>>>> 3 files changed, 31 insertions(+), 14 deletions(-) >>>>> >>>>> diff --git a/examples/configuration/swupdate.cfg b/examples/configuration/swupdate.cfg >>>>> index e8f7dd0..42ec91f 100644 >>>>> --- a/examples/configuration/swupdate.cfg >>>>> +++ b/examples/configuration/swupdate.cfg >>>>> @@ -107,6 +107,8 @@ identify : ( >>>>> # path of the file containing the certificate for SSL connection >>>>> # targettoken : string >>>>> # hawkBit target security token >>>>> +# gatewaytoken : string >>>>> +# hawkBit gateway security token >>>>> # proxy : string >>>>> # in case the server is reached via a proxy >>>>> # ciphers : string in the format used by CURL to set the allowed ciphers suite >>>>> @@ -132,6 +134,7 @@ suricatta : >>>>> sslkey = "/etc/ssl/sslkey"; >>>>> sslcert = "/etc/ssl/sslcert"; >>>>> targettoken = "3bc13b476cb3962a0c63a5c92beacfh7"; >>>>> + gatewaytoken = "99616d4fae39167deddf21cd90047861"; >>>>> */ >>>>> }; >>>>> >>>>> diff --git a/suricatta/server_hawkbit.c b/suricatta/server_hawkbit.c >>>>> index eabb9dd..97e571d 100644 >>>>> --- a/suricatta/server_hawkbit.c >>>>> +++ b/suricatta/server_hawkbit.c >>>>> @@ -62,6 +62,7 @@ static struct option long_options[] = { >>>>> {"retrywait", required_argument, NULL, 'w'}, >>>>> {"proxy", optional_argument, NULL, 'y'}, >>>>> {"targettoken", required_argument, NULL, 'k'}, >>>>> + {"gatewaytoken", required_argument, NULL, 'g'}, >>>>> {NULL, 0, NULL, 0}}; >>>>> >>>>> static unsigned short mandatory_argument_count = 0; >>>>> @@ -1476,7 +1477,8 @@ void suricatta_print_help(void) >>>>> "resume a download (default: %ds).\n" >>>>> "\t -y, --proxy Use proxy. Either give proxy URL, else " >>>>> "{http,all}_proxy env is tried.\n" >>>>> - "\t -k, --targettoken Set target token.\n", >>>>> + "\t -k, --targettoken Set target token.\n" >>>>> + "\t -g, --gatewaytoken Set gateway token.\n", >>>>> DEFAULT_POLLING_INTERVAL, DEFAULT_RESUME_TRIES, >>>>> DEFAULT_RESUME_DELAY); >>>>> } >>>>> @@ -1527,12 +1529,11 @@ static int suricatta_settings(void *elem, void __attribute__ ((__unused__)) *da >>>>> if (strlen(tmp)) >>>>> SETSTRING(channel_data_defaults.proxy, tmp); >>>>> GET_FIELD_STRING_RESET(LIBCFG_PARSER, elem, "targettoken", tmp); >>>>> - if (strlen(tmp)) { >>>>> - char *token_header; >>>>> - if (asprintf(&token_header, "Authorization: TargetToken %s", tmp)) >>>>> - SETSTRING(channel_data_defaults.header, token_header); >>>>> - } >>>>> - >>>>> + if (strlen(tmp)) >>>>> + SETSTRING(server_hawkbit.targettoken, tmp); >>>>> + GET_FIELD_STRING_RESET(LIBCFG_PARSER, elem, "gatewaytoken", tmp); >>>>> + if (strlen(tmp)) >>>>> + SETSTRING(server_hawkbit.gatewaytoken, tmp); >>>>> return 0; >>>>> >>>>> } >>>>> @@ -1591,7 +1592,7 @@ server_op_res_t server_start(char *fname, int argc, char *argv[]) >>>>> >>>>> /* reset to optind=1 to parse suricatta's argument vector */ >>>>> optind = 1; >>>>> - while ((choice = getopt_long(argc, argv, "t:i:c:u:p:xr:y::w:k:", >>>>> + while ((choice = getopt_long(argc, argv, "t:i:c:u:p:xr:y::w:k:g:", >>>>> long_options, NULL)) != -1) { >>>>> switch (choice) { >>>>> case 't': >>>>> @@ -1603,12 +1604,11 @@ server_op_res_t server_start(char *fname, int argc, char *argv[]) >>>>> mandatory_argument_count |= ID_BIT; >>>>> break; >>>>> case 'k': >>>>> - { >>>>> - char *token_header; >>>>> - if (asprintf(&token_header, "Authorization: TargetToken %s", optarg)) >>>>> - SETSTRING(channel_data_defaults.header, token_header); >>>>> - break; >>>>> - } >>>>> + SETSTRING(server_hawkbit.targettoken, optarg); >>>>> + break; >>>>> + case 'g': >>>>> + SETSTRING(server_hawkbit.gatewaytoken, optarg); >>>>> + break; >>>>> case 'c': >>>>> /* When no persistent update state storage is available, >>>>> * use command line switch to instruct what to report. >>>>> @@ -1688,6 +1688,18 @@ server_op_res_t server_start(char *fname, int argc, char *argv[]) >>>>> if (channel_curl_init() != CHANNEL_OK) >>>>> return SERVER_EINIT; >>>>> >>>>> + /* >>>>> + * Set hawkBit token(s) header if any has been set via command line >>>>> + * or configuration file >>>>> + */ >>>>> + char *tokens_header = strdup(""); >>>> >>>> I have tried to imagine why this is required, I do not get it. Can you >>>> explain ? IMHO you are just causing a memory leak because the allocated >>>> string with strdup (just '\0') is never freed. The first asprintf will >>>> overwrite the pointer. >>> >>> I'm sorry if this was not explained, and thanks for pointing out the memleak. >>> >>> The problem we wanted to solve with that code is the case where both >>> target and gateway token are passed by the user. >>> Possible solutions are: >>> 1) put both Auth methods in the HTTP header >>> 2) spit an error and fail >>> 3) spit a warning and put just one of the two (which one??) Auth >>> methods in the HTTP header >>> >>> We've decided to go with option 1) as the hawkBit server could decide >>> which one (of the two Auth methods provided by the client) to use >>> based on the server Authentication Configuration (enabled Auth >>> methods). >> >> What is Hawkbit's behavior if we put both token into the header ? Really >> I do not know. >> >> And what is the behavior if one token is correct and the second one not >> ? Is itauthenticated or not ? >> >> If it is correct that a device can send both tokens, 1) is correct. But >> it can be a nightmare if Hawkbit decides to not authenticate a device if >> one of the token is wrong. >> >> If both tokens are not allowed, we should raise an error and 2) is correct. > > I just discussed on gitter with the hawkBit maintainers and they will > stick with the current hawkBit behaviour of considering just the first > match of the field name (Authentication) in the header provided. ok, thanks for this. > > I will send V2 shortly. > Best regards, Stefano Babic
diff --git a/examples/configuration/swupdate.cfg b/examples/configuration/swupdate.cfg index e8f7dd0..42ec91f 100644 --- a/examples/configuration/swupdate.cfg +++ b/examples/configuration/swupdate.cfg @@ -107,6 +107,8 @@ identify : ( # path of the file containing the certificate for SSL connection # targettoken : string # hawkBit target security token +# gatewaytoken : string +# hawkBit gateway security token # proxy : string # in case the server is reached via a proxy # ciphers : string in the format used by CURL to set the allowed ciphers suite @@ -132,6 +134,7 @@ suricatta : sslkey = "/etc/ssl/sslkey"; sslcert = "/etc/ssl/sslcert"; targettoken = "3bc13b476cb3962a0c63a5c92beacfh7"; + gatewaytoken = "99616d4fae39167deddf21cd90047861"; */ }; diff --git a/suricatta/server_hawkbit.c b/suricatta/server_hawkbit.c index eabb9dd..97e571d 100644 --- a/suricatta/server_hawkbit.c +++ b/suricatta/server_hawkbit.c @@ -62,6 +62,7 @@ static struct option long_options[] = { {"retrywait", required_argument, NULL, 'w'}, {"proxy", optional_argument, NULL, 'y'}, {"targettoken", required_argument, NULL, 'k'}, + {"gatewaytoken", required_argument, NULL, 'g'}, {NULL, 0, NULL, 0}}; static unsigned short mandatory_argument_count = 0; @@ -1476,7 +1477,8 @@ void suricatta_print_help(void) "resume a download (default: %ds).\n" "\t -y, --proxy Use proxy. Either give proxy URL, else " "{http,all}_proxy env is tried.\n" - "\t -k, --targettoken Set target token.\n", + "\t -k, --targettoken Set target token.\n" + "\t -g, --gatewaytoken Set gateway token.\n", DEFAULT_POLLING_INTERVAL, DEFAULT_RESUME_TRIES, DEFAULT_RESUME_DELAY); } @@ -1527,12 +1529,11 @@ static int suricatta_settings(void *elem, void __attribute__ ((__unused__)) *da if (strlen(tmp)) SETSTRING(channel_data_defaults.proxy, tmp); GET_FIELD_STRING_RESET(LIBCFG_PARSER, elem, "targettoken", tmp); - if (strlen(tmp)) { - char *token_header; - if (asprintf(&token_header, "Authorization: TargetToken %s", tmp)) - SETSTRING(channel_data_defaults.header, token_header); - } - + if (strlen(tmp)) + SETSTRING(server_hawkbit.targettoken, tmp); + GET_FIELD_STRING_RESET(LIBCFG_PARSER, elem, "gatewaytoken", tmp); + if (strlen(tmp)) + SETSTRING(server_hawkbit.gatewaytoken, tmp); return 0; } @@ -1591,7 +1592,7 @@ server_op_res_t server_start(char *fname, int argc, char *argv[]) /* reset to optind=1 to parse suricatta's argument vector */ optind = 1; - while ((choice = getopt_long(argc, argv, "t:i:c:u:p:xr:y::w:k:", + while ((choice = getopt_long(argc, argv, "t:i:c:u:p:xr:y::w:k:g:", long_options, NULL)) != -1) { switch (choice) { case 't': @@ -1603,12 +1604,11 @@ server_op_res_t server_start(char *fname, int argc, char *argv[]) mandatory_argument_count |= ID_BIT; break; case 'k': - { - char *token_header; - if (asprintf(&token_header, "Authorization: TargetToken %s", optarg)) - SETSTRING(channel_data_defaults.header, token_header); - break; - } + SETSTRING(server_hawkbit.targettoken, optarg); + break; + case 'g': + SETSTRING(server_hawkbit.gatewaytoken, optarg); + break; case 'c': /* When no persistent update state storage is available, * use command line switch to instruct what to report. @@ -1688,6 +1688,18 @@ server_op_res_t server_start(char *fname, int argc, char *argv[]) if (channel_curl_init() != CHANNEL_OK) return SERVER_EINIT; + /* + * Set hawkBit token(s) header if any has been set via command line + * or configuration file + */ + char *tokens_header = strdup(""); + if (server_hawkbit.targettoken != NULL) + asprintf(&tokens_header, "Authorization: TargetToken %s\n", server_hawkbit.targettoken); + if (server_hawkbit.gatewaytoken != NULL) + asprintf(&tokens_header, "%sAuthorization: GatewayToken %s\n", tokens_header, server_hawkbit.gatewaytoken); + if (strlen(tokens_header)) + SETSTRING(channel_data_defaults.header, tokens_header); + /* * Allocate a channel to communicate with the server */ diff --git a/suricatta/server_hawkbit.h b/suricatta/server_hawkbit.h index f851812..e7cf3dc 100644 --- a/suricatta/server_hawkbit.h +++ b/suricatta/server_hawkbit.h @@ -39,6 +39,8 @@ typedef struct { char *errors[HAWKBIT_MAX_REPORTED_ERRORS]; int errorcnt; const char *update_action; + char *targettoken; + char *gatewaytoken; } server_hawkbit_t; extern server_hawkbit_t server_hawkbit;