| Message ID | 1520605496-19708-2-git-send-email-sbabic@denx.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | None | expand |
Hi Stefano, > After adding a general way to save the stream, Well, that saves the stream, right, but then also immediately installs it as well. This is a change in functionality and I think it removes some flexibility: Consider suricatta wanting to download the file but not install it right away but instead wait for a maintenance window to appear while having downloaded the artifact to spare the time required for downloading while installation. While this was possible before, it isn't after this patch. > drop the hack in suricatta to do the same. I'd rather opt for reintroducing this or some function-similar feature. channel_curl should be generic enough to also cover the above use case, shouldn't it? What do you think? Kind regards, Christian
Hi Christian, On 02/05/2018 13:31, Christian Storm wrote: > Hi Stefano, > >> After adding a general way to save the stream, > > Well, that saves the stream, right, but then also immediately installs > it as well. Well, swupdate is supposed to do its job... > This is a change in functionality and I think it removes > some flexibility: > Consider suricatta wanting to download the file but not install it right > away but instead wait for a maintenance window to appear while having > downloaded the artifact to spare the time required for downloading while > installation. While this was possible before, it isn't after this patch. This was possible, but frankly speaking, it was a hack. Why should the suricatta module blocks the installation without that the main process is informed ? It was like a short-cut in suricatta that does not forward the image, that means it must be aware about what the SWU contains. But suricatta, as web server and the other modules, should not - they fetch the SWU, but they should not process it. Strange enough, there is no check at all because the SWU is not forwarded. The image can be sent by a malicious, and it is stored. This is also a security leak because a very big file can be transferred as result of a DOS attack. I will prefer to add a run dry mode to SWUpdate, where the SWU is forwarded, verified and not installed. This is also in my TO DO list. I think that such as behavior will be compatible with the past and add a major security. A not-signed SWU is dropped as soon as sw-description is not verified, and it is not stored anymore on the local filesystem. > > >> drop the hack in suricatta to do the same. > > I'd rather opt for reintroducing this I am not, or better, not as it was before. > or some function-similar feature. > channel_curl should be generic enough to also cover the above use case, > shouldn't it? What do you think? See above. I would like that in such a case, the SWU is verified as much as possible. Of course, pre- / post- install script are not allowed to run, and a sort of dummy handler should be called instead of real handler. It could be also helpful to run it as CI at the end of the build process and verifies the resulting SWU much more as if we simply call swupdate with -c to run just the parser. Best regards, Stefano
Hi Stefano, > >> After adding a general way to save the stream, > > > > Well, that saves the stream, right, but then also immediately installs > > it as well. > > Well, swupdate is supposed to do its job... > > > This is a change in functionality and I think it removes > > some flexibility: > > Consider suricatta wanting to download the file but not install it right > > away but instead wait for a maintenance window to appear while having > > downloaded the artifact to spare the time required for downloading while > > installation. While this was possible before, it isn't after this patch. > > This was possible, but frankly speaking, it was a hack. Why should the > suricatta module blocks the installation without that the main process > is informed ? Well, that depends, I think: If you see suricatta as master and kind of controlling SWUpdate "core", then it could make sense. If you see it as a simple transport for .swu files, then it maybe doesn't make sense. > It was like a short-cut in suricatta that does not forward > the image, that means it must be aware about what the SWU contains. Well, another option is that suricatta simply doesn't care and inputs the .swu into the SWUpdate core when it needs to without looking into it. Then, of course, with the consequences that the file is broken, corrupt etc. being detected (too) late. > But suricatta, as web server and the other modules, should not - they > fetch the SWU, but they should not process it. > > Strange enough, there is no check at all because the SWU is not > forwarded. The image can be sent by a malicious, and it is stored. This > is also a security leak because a very big file can be transferred as > result of a DOS attack. Yes, true. > I will prefer to add a run dry mode to SWUpdate, where the SWU is > forwarded, verified and not installed. This is also in my TO DO list. This would perfectly fit the use case, much appreciated. > I think that such as behavior will be compatible with the past and add a > major security. A not-signed SWU is dropped as soon as sw-description is > not verified, and it is not stored anymore on the local filesystem. Yes, that would be fine! > >> drop the hack in suricatta to do the same. > > > > I'd rather opt for reintroducing this > > I am not, or better, not as it was before. > > > or some function-similar feature. > > channel_curl should be generic enough to also cover the above use case, > > shouldn't it? What do you think? > > See above. I would like that in such a case, the SWU is verified as much > as possible. Of course, pre- / post- install script are not allowed to > run, and a sort of dummy handler should be called instead of real > handler. It could be also helpful to run it as CI at the end of the > build process and verifies the resulting SWU much more as if we simply > call swupdate with -c to run just the parser. Yes, this would be a welcomed addition, indeed. Kind regards, Christian
Hi Christian, On 02/05/2018 14:12, Christian Storm wrote: > Hi Stefano, > > >>>> After adding a general way to save the stream, >>> >>> Well, that saves the stream, right, but then also immediately installs >>> it as well. >> >> Well, swupdate is supposed to do its job... >> >>> This is a change in functionality and I think it removes >>> some flexibility: >>> Consider suricatta wanting to download the file but not install it right >>> away but instead wait for a maintenance window to appear while having >>> downloaded the artifact to spare the time required for downloading while >>> installation. While this was possible before, it isn't after this patch. >> >> This was possible, but frankly speaking, it was a hack. Why should the >> suricatta module blocks the installation without that the main process >> is informed ? > > Well, that depends, I think: If you see suricatta as master and kind of > controlling SWUpdate "core", then it could make sense. But it is not in the architecture... > If you see it as > a simple transport for .swu files, then it maybe doesn't make sense. Right. suricatta is one of the fetcher for the SWU, and the behavior / interface is the same (the IPC connection) as for Webserver, downloader, etc. The fetcher must not know and must not take any hard decision - in this way, privilege separation works and suricatta / webserver can run with lower rights. If they are starting to become too important, they will need higher privileges. > > >> It was like a short-cut in suricatta that does not forward >> the image, that means it must be aware about what the SWU contains. > > Well, another option is that suricatta simply doesn't care and inputs > the .swu into the SWUpdate core when it needs to without looking into > it. Yes, this is the design. > Then, of course, with the consequences that the file is broken, > corrupt etc. being detected (too) late. If file is corrupt, SWUpdate drops the IPC connection and suricatta is informed. Just in case that SWUpdate is checking the SWU we can detect the corruption very soon. > > >> But suricatta, as web server and the other modules, should not - they >> fetch the SWU, but they should not process it. >> >> Strange enough, there is no check at all because the SWU is not >> forwarded. The image can be sent by a malicious, and it is stored. This >> is also a security leak because a very big file can be transferred as >> result of a DOS attack. > > Yes, true. > > >> I will prefer to add a run dry mode to SWUpdate, where the SWU is >> forwarded, verified and not installed. This is also in my TO DO list. > > This would perfectly fit the use case, much appreciated. Agree - I think it can solve the open points without breaking the use cases. > > >> I think that such as behavior will be compatible with the past and add a >> major security. A not-signed SWU is dropped as soon as sw-description is >> not verified, and it is not stored anymore on the local filesystem. > > Yes, that would be fine! Ok > > >>>> drop the hack in suricatta to do the same. >>> >>> I'd rather opt for reintroducing this >> >> I am not, or better, not as it was before. >> >>> or some function-similar feature. >>> channel_curl should be generic enough to also cover the above use case, >>> shouldn't it? What do you think? >> >> See above. I would like that in such a case, the SWU is verified as much >> as possible. Of course, pre- / post- install script are not allowed to >> run, and a sort of dummy handler should be called instead of real >> handler. It could be also helpful to run it as CI at the end of the >> build process and verifies the resulting SWU much more as if we simply >> call swupdate with -c to run just the parser. > > Yes, this would be a welcomed addition, indeed. > Best regards, Stefano
diff --git a/corelib/channel_curl.c b/corelib/channel_curl.c index 834100d..2de5819 100644 --- a/corelib/channel_curl.c +++ b/corelib/channel_curl.c @@ -66,7 +66,7 @@ channel_op_res_t channel_curl_init(void); channel_op_res_t channel_close(channel_t *this); channel_op_res_t channel_open(channel_t *this, void *cfg); channel_op_res_t channel_get(channel_t *this, void *data); -channel_op_res_t channel_get_file(channel_t *this, void *data, int file_handle); +channel_op_res_t channel_get_file(channel_t *this, void *data); channel_op_res_t channel_put(channel_t *this, void *data); channel_t *channel_new(void); @@ -705,9 +705,10 @@ channel_op_res_t channel_put(channel_t *this, void *data) } } -channel_op_res_t channel_get_file(channel_t *this, void *data, int file_handle) +channel_op_res_t channel_get_file(channel_t *this, void *data) { channel_curl_t *channel_curl = this->priv; + int file_handle; assert(data != NULL); assert(channel_curl->handle != NULL); @@ -745,22 +746,18 @@ channel_op_res_t channel_get_file(channel_t *this, void *data, int file_handle) goto cleanup_header; } - if (file_handle == FD_USE_IPC) { - for (int retries = 3; retries >= 0; retries--) { - file_handle = ipc_inst_start_ext(channel_data->source, - channel_data->info == NULL ? 0 : strlen(channel_data->info), - channel_data->info); - if (file_handle > 0) - break; - sleep(1); - } - if (file_handle < 0) { - ERROR("Cannot open SWUpdate IPC stream: %s\n", strerror(errno)); - result = CHANNEL_EIO; - goto cleanup_header; - } - } else { - assert(file_handle > 0); + for (int retries = 3; retries >= 0; retries--) { + file_handle = ipc_inst_start_ext(channel_data->source, + channel_data->info == NULL ? 0 : strlen(channel_data->info), + channel_data->info); + if (file_handle > 0) + break; + sleep(1); + } + if (file_handle < 0) { + ERROR("Cannot open SWUpdate IPC stream: %s\n", strerror(errno)); + result = CHANNEL_EIO; + goto cleanup_header; } write_callback_t wrdata; diff --git a/corelib/downloader.c b/corelib/downloader.c index 23471f1..7cfd630 100644 --- a/corelib/downloader.c +++ b/corelib/downloader.c @@ -56,7 +56,7 @@ static RECOVERY_STATUS download_from_url(channel_data_t* channel_data) notify(DOWNLOAD, 0, INFOLEVEL, NULL); RECOVERY_STATUS result = SUCCESS; - channel_op_res_t chanresult = channel->get_file(channel, channel_data, FD_USE_IPC); + channel_op_res_t chanresult = channel->get_file(channel, channel_data); if (chanresult != CHANNEL_OK) { result = FAILURE; } diff --git a/include/channel.h b/include/channel.h index f12edc2..0b59eab 100644 --- a/include/channel.h +++ b/include/channel.h @@ -20,7 +20,7 @@ struct channel { channel_op_res_t (*open)(channel_t *this, void *cfg); channel_op_res_t (*close)(channel_t *this); channel_op_res_t (*get)(channel_t *this, void *data); - channel_op_res_t (*get_file)(channel_t *this, void *data, int file_handle); + channel_op_res_t (*get_file)(channel_t *this, void *data); channel_op_res_t (*put)(channel_t *this, void *data); void *priv; }; diff --git a/include/channel_curl.h b/include/channel_curl.h index 0e3ac89..6b5d904 100644 --- a/include/channel_curl.h +++ b/include/channel_curl.h @@ -27,8 +27,6 @@ typedef enum { CHANNEL_PUT, } channel_method_t; -#define FD_USE_IPC -2 - #define USE_PROXY_ENV (char *)0x11 typedef struct { diff --git a/suricatta/server_hawkbit.c b/suricatta/server_hawkbit.c index 6509793..e11777e 100644 --- a/suricatta/server_hawkbit.c +++ b/suricatta/server_hawkbit.c @@ -1068,7 +1068,7 @@ server_op_res_t server_process_update_artifact(int action_id, server_get_current_time(&server_time); channel_op_res_t cresult = - channel->get_file(channel, (void *)&channel_data, FD_USE_IPC); + channel->get_file(channel, (void *)&channel_data); if ((result = map_channel_retcode(cresult)) != SERVER_OK) { /* this is called to collect errors */ ipc_wait_for_complete(server_update_status_callback);
After adding a general way to save the stream, drop the hack in suricatta to do the same. Signed-off-by: Stefano Babic <sbabic@denx.de> --- corelib/channel_curl.c | 33 +++++++++++++++------------------ corelib/downloader.c | 2 +- include/channel.h | 2 +- include/channel_curl.h | 2 -- suricatta/server_hawkbit.c | 2 +- 5 files changed, 18 insertions(+), 23 deletions(-)