Update error handling and debug logging

mamckee commented 1 month ago

This PR primarily serves to make SymCrypt-OpenSSL more debuggable, especially in the provider.

Refactored existing error messages in SymCryptEngine and ScosslCommon to match current function names
Added error functions for the SymCrypt provider
Surface SymCrypt errors through SCOSSL_LOG_SYMCRYPT_ERROR in the provider
Added config options in the SymCrypt provider for logging level and file
Added ERR_set_mark and ERR_pop_to_mark to RSA and ECC KeysInUse init to avoid surfacing KeysInUse errors to the caller

samuel-lee-msft commented 1 month ago

        key,

Wondering whether we should have a SCOSSL_LOG_INFO, SCOSSL_ERR_R_NOT_FIPS_ALGORITHM here for the no padding case #Resolved

Refers to: ScosslCommon/src/scossl_rsa.c:503 in f0f1b9b. [](commit_id = f0f1b9b5691802ec1949d79c20bf514c6aeffb4f, deletion_comment = False)

samuel-lee-msft commented 1 month ago

        ERR_raise(ERR_LIB_PROV, ERR_R_INIT_FAIL);

Should these be SCOSSL_LOG_ERRORs? #Resolved

Refers to: SymCryptProvider/src/p_scossl_base.c:566 in e4e4818. [](commit_id = e4e48181dd1dc6856c267cf19bbe183150d171fc, deletion_comment = False)

samuel-lee-msft commented 1 month ago

        ERR_put_error(_scossl_err_library_code, func_code, reason_code, file, line);

I see this has been deprecated in OpenSSL 3.0 - with ERR_raise (not directly specifying func, file and line) being preferred.

I guess if ERR_put_error is not available, we need to directly invoke the underlying ERR_new / ERR_set_debug / ERR_set_error here? #Resolved

Refers to: ScosslCommon/src/scossl_helpers.c:296 in e4e4818. [](commit_id = e4e48181dd1dc6856c267cf19bbe183150d171fc, deletion_comment = False)

samuel-lee-msft commented 1 month ago

        ERR_raise(ERR_LIB_PROV, ERR_R_INIT_FAIL);

I guess - general question; should there be any ERR_raise in the provider code, or should we route it all through SCOSSL_LOG* macros?

In reply to: 2359556181

Refers to: SymCryptProvider/src/p_scossl_base.c:566 in e4e4818. [](commit_id = e4e48181dd1dc6856c267cf19bbe183150d171fc, deletion_comment = False)

samuel-lee-msft commented 1 month ago

define SCOSSL_LOG_SYMCRYPT_ERROR(func_code, reason_code, description, scError) \

Should we remove this parameter and hardcode to SCOSSL_ERR_R_SYMCRYPT_FAILURE? (for all SCOSSL_LOG_SYMCRYPT*) #Resolved

Refers to: ScosslCommon/inc/scossl_helpers.h:313 in e4e4818. [](commit_id = e4e48181dd1dc6856c267cf19bbe183150d171fc, deletion_comment = False)

mamckee commented 1 month ago

        ERR_raise(ERR_LIB_PROV, ERR_R_INIT_FAIL);

In my experience, relying on the OpenSSL error stack for debugging has been sufficient, since the provider error codes are descriptive and ERR_raise includes the function and line number. I think it would only be necessary if we found a use case where the error stack was ignored by an application in case of failure, but I'm yet to see that.

I think there would be value to add SCOSSL_LOG_ERROR when the error code is ambiguous. This case or any of the internal errors, where some extra description is necessary.

In reply to: 2359585467

Refers to: SymCryptProvider/src/p_scossl_base.c:566 in e4e4818. [](commit_id = e4e48181dd1dc6856c267cf19bbe183150d171fc, deletion_comment = False)

mamckee commented 1 month ago

        ERR_put_error(_scossl_err_library_code, func_code, reason_code, file, line);

Yeah, that's not ideal but we should move off of ERR_put_error then.

In reply to: 2359584794

Refers to: ScosslCommon/src/scossl_helpers.c:296 in e4e4818. [](commit_id = e4e48181dd1dc6856c267cf19bbe183150d171fc, deletion_comment = False)

mamckee commented 1 month ago

define SCOSSL_LOG_SYMCRYPT_ERROR(func_code, reason_code, description, scError) \

That makes sense to me. I also noticed the parameters on SCOSSL_LOG_BIGNUM_ERROR and SCOSSL_LOG_BIGNUM_INFO are wrong, but we don't use them anywhere. Is there any reason to keep either around?

In reply to: 2359587202

Refers to: ScosslCommon/inc/scossl_helpers.h:313 in e4e4818. [](commit_id = e4e48181dd1dc6856c267cf19bbe183150d171fc, deletion_comment = False)

samuel-lee-msft commented 1 month ago

define SCOSSL_LOG_SYMCRYPT_ERROR(func_code, reason_code, description, scError) \

Good catch - given SCOSSL_LOG_BIGNUM* is no longer being used, I think it's reasonable to remove it and _scossl_log_bignum

In reply to: 2359608976

Refers to: ScosslCommon/inc/scossl_helpers.h:313 in e4e4818. [](commit_id = e4e48181dd1dc6856c267cf19bbe183150d171fc, deletion_comment = False)

mamckee commented 1 month ago

        ERR_put_error(_scossl_err_library_code, func_code, reason_code, file, line);

In the process of doing this, I remembered that the function code is ignored in OpenSSL 3. We'd have to copy a significant amount of the error logging code from 1.1.1 to reintroduce this, but I don't think it adds anything for our provider code. I think the provider should just use SCOSSL_LOG macros that don't accept a function code but log everything else the same.

In reply to: 2359605816

Refers to: ScosslCommon/src/scossl_helpers.c:296 in e4e4818. [](commit_id = e4e48181dd1dc6856c267cf19bbe183150d171fc, deletion_comment = False)

microsoft / SymCrypt-OpenSSL