[Pal/lib] Remove `list-test`, move `path.c`, refactor crypto code #2367

dimakuv · 2021-05-11T12:21:07Z

Description of the changes

One more PR in the series of Pal/lib refactoring.

Three commits here:

[Pal/lib] Remove unused list-test.c
[Pal/lib] Move Pal/lib/graphene/path.c to Pal/lib/path.c. This is to get rid of the graphene/ subdir. All Graphene-specific utility files are stored under Pal/lib/ by convention.
[Pal/lib] Remove redundant functionality from crypto. This commit simplifies the crypto code:
- rng-arch.h header and its get_rand64() are replaced with _DkRandomBitsRead() and subsequently removed.
- lib_Base64Encode() and lib_Base64Decode() are replaced with their mbedTLS counterparts and subsequently removed.
- All mbedTLS wrappers are amalgamated in a single mbedtls_adapter.c.
- pal_crypto.h header is renamed to crypto.h (it has nothing to do with PAL code).

How to test this PR?

All tests must pass.

This change is

dimakuv

Reviewable status: 0 of 19 files reviewed, all discussions resolved, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel)

Pal/lib/crypto/adapters/mbedtls_adapter.c, line 20 at r1 (raw file):

#include "mbedtls/rsa.h"
#include "mbedtls/sha256.h"
#include "pal_error.h"

FYI: I plan to move and rename this pal_error.h header in a next PR. It doesn't make sense for the "common library" to include PAL-specific headers.

Pal/lib/crypto/adapters/mbedtls_adapter.c, line 423 at r1 (raw file):

    return 0;
}

FYI: Below code is copied from mbedtls_dh.c

dimakuv · 2021-05-11T13:52:38Z

Jenkins, retest Jenkins-Debug-20.04 please (kill11 LTP test failed)

boryspoplawski

Reviewed 16 of 19 files at r1, 3 of 3 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)

Pal/lib/crypto/adapters/mbedtls_adapter.c, line 23 at r2 (raw file):

/* This is declared in pal_internal.h, but that can't be included here. */
int _DkRandomBitsRead(void* buffer, size_t size);

How does this work? Isn't the resulting object file .o included in LibOS binary? Wouldn't that cause LibOS to use PAL internal interface?
(seems to be an existing issue)

dimakuv

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski)

Pal/lib/crypto/adapters/mbedtls_adapter.c, line 23 at r2 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

How does this work? Isn't the resulting object file .o included in LibOS binary? Wouldn't that cause LibOS to use PAL internal interface?
(seems to be an existing issue)

Good question. But not, LibOS will not use _DkRandomBitsRead() because this function is only called from mbedTLS wrappers. LibOS doesn't use these wrappers (in particular, the file mbedtls_adapter.o). So during the final linking step, when LibOS sources are linked against static graphene-lib.a, the linker notices that mbedTLS wrappers are not used and removes all that stuff. So in the end, the libsysdb.so library doesn't have any references to _DkRandomBitsRead().

I verified it by manually inspecting the symbols and relocations in the resulting libraries.

This is a pretty bad design decision though... I don't know yet how to fix it. I could keep it as-is for now. (Maybe it's not too bad, after all it's just a callback that the actual user of this helper lib needs to provide.)

yamahata

Reviewed 16 of 19 files at r1, 3 of 3 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski)

boryspoplawski

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)

Pal/lib/crypto/adapters/mbedtls_adapter.c, line 23 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Good question. But not, LibOS will not use _DkRandomBitsRead() because this function is only called from mbedTLS wrappers. LibOS doesn't use these wrappers (in particular, the file mbedtls_adapter.o). So during the final linking step, when LibOS sources are linked against static graphene-lib.a, the linker notices that mbedTLS wrappers are not used and removes all that stuff. So in the end, the libsysdb.so library doesn't have any references to _DkRandomBitsRead().

I verified it by manually inspecting the symbols and relocations in the resulting libraries.

This is a pretty bad design decision though... I don't know yet how to fix it. I could keep it as-is for now. (Maybe it's not too bad, after all it's just a callback that the actual user of this helper lib needs to provide.)

Please add a comment about it then (GH issue, in code, wherever).

dimakuv

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski)

Pal/lib/crypto/adapters/mbedtls_adapter.c, line 23 at r2 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Please add a comment about it then (GH issue, in code, wherever).

Done. #2371

boryspoplawski

Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners

mkow

Reviewed 16 of 19 files at r1, 3 of 3 files at r2.
Reviewable status: all files reviewed, 6 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @dimakuv)

Pal/lib/crypto/adapters/mbedtls_adapter.c, line 3 at r2 (raw file):

/* SPDX-License-Identifier: LGPL-3.0-or-later */
/* Copyright (C) 2017 Fortanix, Inc. */
/* Copyright (C) 2019, Texas A&M University. */

Could you drop the comma and the period to unify with other places?

Pal/lib/crypto/adapters/mbedtls_adapter.c, line 439 at r2 (raw file):

     * haven't implemented that yet. */
    ret = mbedtls_mpi_read_string(&context->P, 16 /* radix */, MBEDTLS_DHM_RFC3526_MODP_2048_P);
    if (ret < 0)

You changed all != to <, but I think mbedtls returns positive error codes?

Pal/src/host/Linux-SGX/tools/common/attestation.c, line 175 at r2 (raw file):

    ret = mbedtls_base64_decode(ias_sig, ias_sig_size, &ias_sig_size, ias_sig_b64,
                                ias_sig_b64_size);
    if (ret < 0) {

not !=?

Pal/src/host/Linux-SGX/tools/common/attestation.c, line 289 at r2 (raw file):

    ret = mbedtls_base64_decode(report_quote, quote_size, &quote_size, (uint8_t*)node->valuestring,
                                strlen(node->valuestring));
    if (ret < 0) {

ditto

Pal/src/host/Linux-SGX/tools/common/ias.c, line 353 at r2 (raw file):

        ret = mbedtls_base64_decode(*sigrl, *sigrl_size, sigrl_size, (uint8_t*)ias_resp->data,
                                    strlen(ias_resp->data));
        if (ret < 0 || !*sigrl_size) {

ditto

Pal/src/host/Linux-SGX/tools/common/ias.c, line 397 at r2 (raw file):

    ret = mbedtls_base64_encode((uint8_t*)quote_b64, quote_b64_size, &quote_b64_size, quote,
                                quote_size);
    if (ret < 0) {

not !=?

dimakuv

Reviewable status: 18 of 19 files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @mkow, and @yamahata)

Pal/lib/crypto/adapters/mbedtls_adapter.c, line 3 at r2 (raw file):