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

Single source for SSL / mbedtls buffer size setting #2248

Merged
merged 1 commit into from
Feb 7, 2018

Conversation

devsaurus
Copy link
Member

@devsaurus devsaurus commented Feb 2, 2018

Addresses #1707.

  • This PR is for the dev branch rather than for master.
  • This PR is compliant with the other contributing guidelines as well (if not, please describe why).
  • I have thoroughly tested my contribution.

Commit "update espconn apps for lwip and mbedtls from non-os sdk master branch"

Recent bug fixes for espconn layer in Espressif's NON-OS SDK appear to fix the HTTP client: Disconnected with error: 8 error with our http client (ref. #1707 (comment)).
espconn* files are copied from third_party/mbedtls and third_party/lwip in https://github.com/espressif/ESP8266_NONOS_SDK/tree/cab958da920a9c29b09b8c8f906780a5d3d0427b/

Commit "enforce single source for SSL buffer size"

We had some inconsistency in the code base related to SSL buffer size configuration.

Situation up to now

  • user_config.h defines SSL_BUFFER_SIZE to 5120
  • app/user/user_main.c uses this to configure the espconn/mbedtls layer
  • But espconn_secure_set_size() is not effective due to our own integration of mbedtls. Espressif applied some tweaks to enable dynamic buffer sizing.
  • In parallel, the real buffer size is defined by MBEDTLS_SSL_MAX_CONTENT_LEN in user_mbedtls.h.

Change

SSL buffer sizing mechanism is (almost) back to pre-mbedtls status:
Single source for SSL/TLS buffer size definition with SSL_BUFFER_SIZE in user_config.h. No need anymore to set the same value in user_mbedtls.h.

Summary

We lost dynamic buffer sizing when we moved to our local mbedtls. It's not possible with the current configuration of mbedtls since the feature MBEDTLS_SSL_MAX_FRAGMENT_LENGTH and dynamic buffers are mutually exclusive due to non-constant initializer element error from app/mbedtls/library/ssl_tls.c:150.
If we decide to disable MBEDTLS_SSL_MAX_FRAGMENT_LENGTH then we could apply the same tweak like Espressif did and add a function to the tls module API to set buffer size during runtime.

@@ -430,12 +434,14 @@ espconn_Task(os_event_t *events)
break;
case SIG_ESPCONN_ERRER:
/*remove the node from the client's active connection list*/
espconn_list_delete(&plink_active, task_msg);
if (espconn_manual_recv_enabled(task_msg))
espconn_list_delete(&plink_active, task_msg);
Copy link
Member

Choose a reason for hiding this comment

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

This looks like dubious indentation, post-patch. I believe that the if and list_delete should be 4 spaces further to the right but that the code is plausibly conveying the correct semantics as written?

espconn_tcp_reconnect(task_msg);
break;
case SIG_ESPCONN_CLOSE:
/*remove the node from the client's active connection list*/
espconn_list_delete(&plink_active, task_msg);
if (espconn_manual_recv_enabled(task_msg))
espconn_list_delete(&plink_active, task_msg);
espconn_tcp_disconnect_successful(task_msg);
Copy link
Member

Choose a reason for hiding this comment

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

Another dubious indent.

TLSmsg = pinfo->pssl;
lwIP_REQUIRE_ACTION(TLSmsg,exit,);

Copy link
Member

Choose a reason for hiding this comment

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

Please do not apply the next hunks, from here to line 452; doing so undoes a deliberate change. See ef7a694 .

@@ -712,12 +716,6 @@ static bool mbedtls_msg_info_load(mbedtls_msg *msg, mbedtls_auth_info *auth_info
}
}

static void
Copy link
Member

Choose a reason for hiding this comment

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

Please do not remove this function.

Copy link
Member

@nwf nwf left a comment

Choose a reason for hiding this comment

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

Updating espconn is a great idea, but this is an over-eager commit. Please see my commentary on #2225 and please cherry-pick commits from Espressif, rather than replacing files wholesale.

@@ -798,7 +796,7 @@ static bool mbedtls_msg_config(mbedtls_msg *msg)
mbedtls_ssl_conf_authmode(&msg->conf, MBEDTLS_SSL_VERIFY_NONE);
}
mbedtls_ssl_conf_rng(&msg->conf, mbedtls_ctr_drbg_random, &msg->ctr_drbg);
mbedtls_ssl_conf_dbg(&msg->conf, mbedtls_dbg, NULL);
Copy link
Member

Choose a reason for hiding this comment

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

Please do not apply this hunk. See b382291 .

Copy link
Member

@nwf nwf left a comment

Choose a reason for hiding this comment

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

The second commit. for buffer sizes, seems fine. (Github's UI leaves a lot to be desired.)

@devsaurus
Copy link
Member Author

Thanks for the review @nwf.

Do you see a possibility to merge your changes for espconn_mbedtls.c back into upstream non-os sdk? That would simplify maintenance a lot.

@nwf
Copy link
Member

nwf commented Feb 3, 2018

I hadn't really considered pushing them upstream, but perhaps they should go that way. Is it just "open a PR" on the SDK repo?

That said, rather than landing a monolithic "update to new SDK" commit, it might be useful for the nodemcu tree to cherrypick or merge commits from Espressif, so that the commit log is more useful, separately from purely preserving our own changes.

@devsaurus
Copy link
Member Author

it might be useful for the nodemcu tree to cherrypick or merge commits from Espressif

I agree, but how to perform that - technically?
A plain git cherry-pick won't work, I assume.

@marcelstoer
Copy link
Member

Is it just "open a PR" on the SDK repo?

Yes, but it might take months to get accepted. Espressif uses the GitHub repo as a kind-of replication of their internal repo.

A plain git cherry-pick won't work, I assume.

I guess you'd have to add the Espressif repo as an additional Git remote first.

@nwf
Copy link
Member

nwf commented Feb 6, 2018

Whoever set up the nodemcu repo did not do so by forking the sdk, so git whines that there's no common ancestry. Upstreaming our changes is probably the simplest step by far, but failing that, the thing to do is probably to extract the diffs from the sdk and mangle them to apply to nodemcu. One hopes that this is easy, given that there are 70 commits between v2.1.0 and v2.2.0.

@nwf
Copy link
Member

nwf commented Feb 6, 2018

I've started reviewing the commits one by one. Here's what I've got for the first several:

  • cba173c259ee89f23b2c1b4fe3cc3339cd1e8ddc superseded by existing changes
  • 10138bdc0e37e4c364401de70e9ac402a1c3de55 messy; updates .a-s. example/ not applicable
  • 6ac645da1d919ba1eb9dc493a8585ffa9ad01868 not applicable
  • dd69ea8340924b344c801926af4bc7cd74bbe5de updates *.a-s
  • 141371b7837c6b356552025c8e4079969ce38344 updates *.a-s
  • 18260135dfd17f40a6e0afc614bc7ce8d997c270 updates *.a-s, change to user_interface.h not applicable to nodemcu tree
  • 33d4ee252ab8b63e47386e5cb375ea95b3a08af5 updates linker scripts; not applicable?
  • b8fd588a33f0319dc135523b51655e97b483b205 updates linker scripts and *.a-s
  • cf83aba8a5f0a44ce39acb8642904f1d52ce876b looks to just move lwip, and has a purely formatting change to espconn_udp.c (which we might consider applying, to reduce deltas)
  • 89ba08ee24c9638dc59d61cfd2ef2819f511bfb9 imports mbedtls; not applicable?
  • 9d41059954132430a3e95851adba5cac0a889336 should be applied
  • 509eae8515793ec62f6501e2783c865f9a8f87e3 not applicable; app/Makefile already sets -DICACHE_FLASH
  • 10aea1782b2cac518a1a30eb8b4e046ed76c8d7c updates *.a-s
  • 990246645199b31807482ed23f7009e8dc53394b updates *.a-s, contains possibly meaningful changes to third_party/include/arch/cc.h and third_party/include/lwip/app/espconn.h .
  • b762ea222ee94b9ffc5e040f4bf78dd8ba4db596 updates *.a-s
  • 33f234f4a6defafec2f9442dcc0bb1ebdb80c42f updates *.a-s
  • cba0686be85102831b2684bccf4c44f868de0906 updates *.a-s
  • 5ee8215c25e9b0ec1dcfcba13820022943c19158 should be applied to local lwip tree.

The ones that just update *.a files are presumably of little importance to us, since we grab the SDK wholesale, including the .a files. Yes?

@marcelstoer
Copy link
Member

Arnim, do you want proceed with this PR standalone or would you rather "embed" this into #2225?

@devsaurus
Copy link
Member Author

do you want proceed with this PR standalone or would you rather "embed" this into #2225?

The latter, I guess. The SDK release surfaced earlier and the topic is more complex than I expected.

I'll remove the problematic commit and reduce this PR to the "single-source for SSL buffer topic". This is independent of any SDK and already agreed upon 😃

@devsaurus devsaurus changed the title Update espconn layer from pre-2.2 SDK Single source for SSL / mbedtls buffer size setting Feb 7, 2018
@marcelstoer marcelstoer added this to the 2.2 milestone Feb 7, 2018
@marcelstoer marcelstoer merged commit e3807bd into nodemcu:dev Feb 7, 2018
@devsaurus devsaurus deleted the espconn_update branch February 11, 2018 12:37
@devsaurus devsaurus mentioned this pull request Feb 11, 2018
dnc40085 pushed a commit to dnc40085/nodemcu-firmware that referenced this pull request Mar 3, 2018
vsky279 pushed a commit to vsky279/nodemcu-firmware that referenced this pull request Mar 10, 2018
@eyaleb
Copy link

eyaleb commented Aug 19, 2018

I am attempting to shove my app into an old esp-07 (512KB flash). It worked fine until the recent SDK 2.2 move but now the image it too large, leaving only 30KB for SPIFFS.

I looked around for ways to trim the image and commented out in user_config.h
//#define SSL_BUFFER_SIZE 5120
Just in case a buffer is actually reserved in the image (unlikely). SSL is disabled.

This broke the build

 #if MBEDTLS_SSL_MAX_CONTENT_LEN > 16384
     ^
../../include/user_mbedtls.h:304:49: error: "SSL_BUFFER_SIZE" is not defined [-Werror=undef]
 #define MBEDTLS_SSL_MAX_CONTENT_LEN             SSL_BUFFER_SIZE /**< Maxium fragment length in bytes, determines the size of each of the two internal I/O buffers */
                                                 ^
../../include/mbedtls/ssl_internal.h:143:35: note: in expansion of macro 'MBEDTLS_SSL_MAX_CONTENT_LEN'
 #define MBEDTLS_SSL_PAYLOAD_LEN ( MBEDTLS_SSL_MAX_CONTENT_LEN    \
                                   ^
../../include/mbedtls/ssl_internal.h:158:5: note: in expansion of macro 'MBEDTLS_SSL_PAYLOAD_LEN'
 #if MBEDTLS_SSL_PAYLOAD_LEN > 16384 + 2048
     ^
cc1: all warnings being treated as errors

FYI

@nwf
Copy link
Member

nwf commented Aug 19, 2018

Please don't resurrect old PRs as a means to get support. Yes, some buffer size is required, so simply commenting out the buffer size definition will not work. Moreover, the buffer is in RAM, not flash, so pruning it won't do what you want. Yet still, if SSL is disabled, none of this code is linked into the final build product. The 512K flash modules are supported only on the old 1.5.4.1 branch of NodeMCU.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants