text:
Quest v0.1 / G6PD: Publisher based Questionnaire sync

Is your feature request related to a problem? Please describe.
As a developer, I want to be able to sync and trigger Questionnaires based on a variable assigned to the logged in user and variables assigned to Questionnaires.
Describe the solution you'd like

Assign a publisher to users (eg somewhere in the user profile we list their publisher) and then when that user logs in we sync any Questionnaires that have the same publisher, so we’re using the publisher to define the “app”.

Start with a "g6pd" publisher and a user with "g6pd" in their profile.

Assign category to describe the function of the Questionnaire

To start we can have a “registration” category, see #567

Describe alternatives you've considered

Sync all Questionnaires for everyone
Use the context field in Questionnaire, doesn't seem as good a fit
Hard code IDs (what we currently do)

Additional context

See #567 for how we will use the category field

From Mobile and FHIR protocol stand point, yes we can filter Questionnaires by context, publisher, jurisdiction, profile, tag etc (see others).
However, we need to see how user/practitioner can be assigned these params and send with profile details from server. @dubdabasoduba . If any of these are sent with profile we can use and send that as filter for sync/search for Questionnaire

Hi @maimoonak. I would like to support with this issue if I can.
You seem to have a good grip on the requirements.
I will appreciate a quick intro to the fhircore app and a briefing on the issue if you have some time this afternoon or tomorrow morning.

@pld @f-odhiambo So @maimoonak very graciously gave me a bit of run through of what is required here.
What we discussed was a incremental solution:

Filter Questionnaires by publisher=g6pd hard-coded.
Persist user publisher in shares preferences.
Filter Questionnaire by persisted user publisher preference.

Please share any comments or thoughts.
@f-odhiambo Please would you assist in connecting me with the team members tackling the server component of this task so that I can collaborate with them?

That sounds reasonable. Do you have any thoughts on using category to manager where the app should load a specific Questionnaire?

That sounds reasonable. Do you have any thoughts on using category to manager where the app should load a specific Questionnaire?

@pld Unless I am missing something, Questionnaire doesn't have a category field.
Do you not think that perhaps identifier would be a better fit?

Ah good point, @maimoonak did I mishear you, I thought you mentioned another field to hold info in on the call?
I wonder what the best place to store additional metadata that represents where in a workflow to load the Questionnaire? Identifier certainly seems like a contender, looking at the resource, I've listed some field options below:

identifier (multiple values)
publisher (single value)
useContext (multiple values)
code (multiple values)

In the longer-term I could image not wanting to use publisher, since it only stores a single value it won't give us flexibility if we want to define multiple more granular attributes for how to integrate a Questionnaire into the app. I could also imaging publishers, like the WHO, etc wanting to put their name in the publisher field.
useContext could be nice, in that we could store an Organization (or Group) as a valueReference and then link users to Questionnaires by putting them in the same Org (or Group). But I'm not sure how we'd use it to specify where in the workflow to load the Questionnaire, but I guess a stripped down HealthcareService might work? For example, the patient registration for for G6PD might have a useContext value of:
[
  {
    code: 'app'
    value:
      {
        valueReference: [the G6PD organization]
      }
  },
  {
    code: 'function`
    value:
      {
        valueReference: [a HealthcareService with only a name field with value "registration"]
      }
  }
]

any other suggestions here?

From Mobile and FHIR protocol stand point, yes we can filter Questionnaires by context, publisher, jurisdiction, profile, tag etc (see others). However, we need to see how user/practitioner can be assigned these params and send with profile details from server. @dubdabasoduba . If any of these are sent with profile we can use and send that as filter for sync/search for Questionnaire

https://github.com/opensrp/web/issues/585 This describes the current practitioner payload that is created from the web.

So we had a conversation including @f-odhiambo @ndegwamartin @maimoonak @dubdabasoduba on how to represent the publisher on the server. Our conclusion was to use user roles in keycloak. @dubdabasoduba Did raise some concerns about the scalability of the approach in the context of the single apk approach and requested that some additional thinking been done on this.
So the implementation that I am proceeding with for now will be to extract the publisher from the user roles and filter the Questionnaires based on the value of that publisher role.
Please let us know if there are any questions or concerns.

Ah good point, @maimoonak did I mishear you, I thought you mentioned another field to hold info in on the call?
I wonder what the best place to store additional metadata that represents where in a workflow to load the Questionnaire? Identifier certainly seems like a contender, looking at the resource, I've listed some field options below:
1. identifier (multiple values)
2. publisher (single value)
3. useContext (multiple values)
4. code (multiple values)

In the longer-term I could image not wanting to use publisher, since it only stores a single value it won't give us flexibility if we want to define multiple more granular attributes for how to integrate a Questionnaire into the app. I could also imaging publishers, like the WHO, etc wanting to put their name in the publisher field.
useContext could be nice, in that we could store an Organization (or Group) as a valueReference and then link users to Questionnaires by putting them in the same Org (or Group). But I'm not sure how we'd use it to specify where in the workflow to load the Questionnaire, but I guess a stripped down HealthcareService might work? For example, the patient registration for for G6PD might have a useContext value of:
[
  {
    code: 'app'
    value:
      {
        valueReference: [the G6PD organization]
      }
  },
  {
    code: 'function`
    value:
      {
        valueReference: [a HealthcareService with only a name field with value "registration"]
      }
  }
]

any other suggestions here?

@pld @maimoonak If we are using the publisher to represent the author/organization that created/designed the Questionnaire which I think it the right way to go, I don't think that it should be used to represent the stage in the workflow. I think that useContext sounds like the correct field to me.

@dubdabasoduba for the concern you raised earlier about alignment of the fhircore vs quest auth flow for questionnaires in relation to the Single APK approach, I think we can solve it by only syncing down questionnaires based on the module configuration . i.e. at the point where we have the configurations for which modules (anc, eir e.t.c) the specific deployment should have, we should also have configurations for the specific (Questionnaire) resources they can sync down.
Alternatively we could somehow extrapolate that from the configured modules and figure out which questionnaires to sync down to the device. I'm thinking this would be at installation and also subsequent syncs as assignments might be dynamic
Let me know your thoughts on this

I guess another important question would be do we envision a situation where the quest mode and the Single APK mode are not mutually exclusive? cc @pld @f-odhiambo

I guess another important question would be, do we envision a situation where the Quest mode and the Single APK mode are not mutually exclusive? cc @pld @f-odhiambo

@ndegwamartin yes I think that is possible but we don't enough information, we discussed this a bit in our Quest architecture call, I see the work we're doing now as a way to see if Quest can be the single app, if we can get it configurable enough to be that.
@trevorgowing @dubdabasoduba I also think we should go with useContext to keep publisher available for it's semantically appropriate values -- we can delay that switch if we're far enough down the path that this adds too much work

@pld @ndegwamartin @dubdabasoduba Just a heads up. We had to configure keycloak to expose the roles in the userInfo endpoint. We have done that now for the FHIR_Android realm as you can see below.
{
    "sub": "dbde5c0c-2aa0-4278-a505-f6ef9d262a1b",
    "email_verified": false,
    "realm_access": {
        "roles": [
            "g6pd-questionnaire",
            "offline_access",
            "default-roles-fhir_android",
            "uma_authorization",
            "g6pd"
        ]
    },
    "name": "G6PD Test",
    "groups": [
        "g6pd-questionnaire",
        "offline_access",
        "default-roles-fhir_android",
        "uma_authorization",
        "g6pd"
    ],
    "preferred_username": "g6pd",
    "given_name": "G6PD",
    "family_name": "Test",
    "email": "g6pddemo@ona.io"
}

@f-odhiambo I can't see how any UI would relate to this issue.
I would expect that a list of Questionnaires would be displayed, all I am doing is filtering the list.

Agree, any UI for this is handled by #585

@syedowaisali90 Please feel free to reach out if you have any questions etc.

I meant to say he will need it 😊 to view synced questionnaires

PR 590 Ready for Review.

authors: dubdabasoduba f-odhiambo maimoonak ndegwamartin pld trevorgowing

repo: opensrp/fhircore


text:
Why EVP_ digest functions depend on pthread?

For hashing, I used until now the old deprecated functions declared in md5.h.
Now I am trying to switch to EVP_ functions but my project does not compile if I don't link with -pthread.
I see that it needs it for some locking
/home/tawfic/Desktop/openssl-OpenSSL_1_1_1g/output/lib/libcrypto.a(threads_pthread.o): In function `CRYPTO_THREAD_lock_new':
/home/tawfic/Desktop/openssl-OpenSSL_1_1_1g/crypto/threads_pthread.c:34: undefined reference to `pthread_rwlock_init'
/home/tawfic/Desktop/openssl-OpenSSL_1_1_1g/output/lib/libcrypto.a(threads_pthread.o): In function `CRYPTO_THREAD_read_lock':
/home/tawfic/Desktop/openssl-OpenSSL_1_1_1g/crypto/threads_pthread.c:65: undefined reference to `pthread_rwlock_rdlock'
/home/tawfic/Desktop/openssl-OpenSSL_1_1_1g/output/lib/libcrypto.a(threads_pthread.o): In function `CRYPTO_THREAD_write_lock':
/home/tawfic/Desktop/openssl-OpenSSL_1_1_1g/crypto/threads_pthread.c:78: undefined reference to `pthread_rwlock_wrlock'
/home/tawfic/Desktop/openssl-OpenSSL_1_1_1g/output/lib/libcrypto.a(threads_pthread.o): In function `CRYPTO_THREAD_unlock':
/home/tawfic/Desktop/openssl-OpenSSL_1_1_1g/crypto/threads_pthread.c:91: undefined reference to `pthread_rwlock_unlock'
/home/tawfic/Desktop/openssl-OpenSSL_1_1_1g/output/lib/libcrypto.a(threads_pthread.o): In function `CRYPTO_THREAD_lock_free':
/home/tawfic/Desktop/openssl-OpenSSL_1_1_1g/crypto/threads_pthread.c:107: undefined reference to `pthread_rwlock_destroy'

Why a simple function like md5 hash needs locking? Is there a documetation on this that I can read?
Thanks

I assume you are using static linking?
Probably there isn't anything directly in the MD5 implementation that requires locking...it is probably coming from some internal dependency. For example locks are used in the error queue management. They are also used in some of the memory management functions.
If your application is not multi-threaded, then one option is to compile OpenSSL with no-threads.

Hi @mattcaswell , thanks for the fast reply.
I am using static linking, yes.
I just did a very small test app that only hashes a string and it seems that I have to link pthread when using EVP.
When using the deprecated functions, there is no need for pthread.

I just did a very small test app that only hashes a string and it seems that I have to link pthread when using EVP.

Right - this is unsurprising. Like I said this is due to the various internal dependencies (such as the error management) that do use locking. If this is a problem for you then compile OpenSSL without thread support as noted above.

I see, it's not a problem for me, I just want to make sure that the EVP functions are not slower than the deprecated ones because of locking :)

I see, it's not a problem for me, I just want to make sure that the EVP functions are not slower than the deprecated ones because of locking :)

I can't think of any locks that you would run into as part of normal successful hashing.

source: gharchive/issue

authors: mattcaswell tawmoto

repo: openssl/openssl

license_type: permissive

license_source: github-api


text:
dgst command should use provider-keymgmt functions for dealing with private keys

I am currently writing my own provider implementation to wrap the access to an external signature server.
Here the private key used for signature is not accessible.
Unfortunately the openssl dgst app always uses the OSSL_OP_STORE/provider-storemgmt functions and not the OSSL_OP_KEYMGMT/provider-keymgmt functions (e.g. OSSL_FUNC_keymgmt_load()) to load and pass private keys.  As result the private key must be always loadable from file system and cannot be just an abstract reference.

Store can be anything, it does not necessarily have to be a file system store. Store handles URIs - if you invent a new URI scheme for your provider you can make it called via the store call with the URI scheme.
See for example the recently added winstore store provider that adds windows certificate store support.
You need to reference the key in one way or another anyway.

But in any case I have to provide the byte sequence of a real private key  to make the apps.c:load_key() function happy?

No, you don't. The store can create a key reference that is valid within your provider only.

And you do not need to provide a keymgmt export function.


text:
Why is X509_V_FLAG_PARTIAL_CHAIN flag required, should it be default?

If you want to explicitly trust an intermediate certificate for certificate chain verification, you have to specify X509_V_FLAG_PARTIAL_CHAIN otherwise the leaf certificate is not verified. In practice any other major TLS library behaves like this by default not requiring any such extra verification flags.
What is the reason for having this as non-default behavior and should it be changed in the future major OpenSSL release?

In practice any other major TLS library behaves like this by default not requiring any such extra verification flags.

Yes, other libraries behave like X509_V_FLAG_PARTIAL_CHAIN out of the box. GnuTLS is one of them.
Other programs behave like X509_V_FLAG_PARTIAL_CHAIN out of the box, too. Wget is one of them.
cURL is getting ready to change because it supports both OpenSSL and GnuTLS backends. cURL is testing changes to set X509_V_FLAG_PARTIAL_CHAIN when using OpenSSL backend so the same behavior occurs when using GnuTLS and OpenSSL. Also see cURL PR 4655, openssl: set X509_V_FLAG_PARTIAL_CHAIN.

@t8m
Maybe this is "fixed" by 02a2567173a451d8d00c276e6d8c1d1cb171234d now? ;-)

No, although that commit is remotely related I do not think this is fixed by it.

Maybe @vdukhovni knows why we have this flag and that it's not the default behaviour.

Maybe @vdukhovni knows why we have this flag and that it's not the default behaviour.

I can answer part of that. I know the reason for the flag, but I don't know why it is not enabled by default.
The partial chain flag is required because RFC 4158, Certification Path Building, requires a certificate path that is rooted to a Root CA. RFC 4158 does not allow an intermediate CA to be the root of trust. Root CA's have "Issuer == Subject", "CA ==true", critical and "spki==akid".
X509_V_FLAG_PARTIAL_CHAIN allows us to use OpenSSL and root trust in a non-root CA, like Let's Encrypt Authority R3. I use X509_V_FLAG_PARTIAL_CHAIN often. For example, I used to patc cURL and Wget that sets X509_V_FLAG_PARTIAL_CHAIN in the OpenSSL connection so I could:
curl --cacert=lets-encypt-r3.pem ...
wget --ca-certificate=lets-encypt-r3.pem ...

(cURL and Wget have since taken the patches to use X509_V_FLAG_PARTIAL_CHAIN).
I despise using the CA Zoo, like Mozilla's cacert.pem or Google's roots.pem. I won't use it unless I have to.
For what it is worth, GnuTLS always used the equivalent to X509_V_FLAG_PARTIAL_CHAIN. That is why a connection using OpenSSL's backend would sometimes fail with cURL and Wget, but a connection using GnuTLS backend would succeed with cURL and Wget.

Another reason that it's not the default (implied by @noloader's comment) is because that would be a behavior change.  Look at https://github.com/openssl/openssl/pull/4912/files to get the diffs, for example.
Please be very careful, and do not change the verification behavior to be "more correct" (quotes on purpose).  Many organizations depend on the current, documented, behavior.

Maybe @vdukhovni knows why we have this flag and that it's not the default behaviour.

It is not accurate that the flag is required in order to trust intermediate CAs. If there's a problem, it is actually that self-signed certificates in the trust store are treated as trust-anchors by default!
The correct way to designate a trust-anchor is to encase the certificate in a "TRUSTED CERTIFICATE" structure, which designates the set of associated trust EKUs. This is akin to what you see in the trust stores in Firefox, or Apple's and Microsoft's certificate stores, ...
The self-signed => full trust policy is regrettable.  All trust-anchors should be explicitly labeled, and when that happens, they need not be self-signed. Indeed OpenSSL's chain building stops at the first trusted certificate.  In OpenSSL we don't have separation between the intermediate CA store and the trust-anchor store, and we do have support for explicit trust-settings, they're under-utilised to the point of most users being completely unaware of them.  In part this is a result of the distributed trust "bundles" not having any explicit trust settings.
Ideally, we'd not dig the hole deeper, and would move towards encouraging explicit trust.  In practice that's difficult, but in any case I don't see a strong case for enabling partial chain by default.  If the intermediate is in the store, why not also include its issuers?  The only case that would not be covered is explicit EE certs in the trust store, which require the same flag.
Perhaps there should be a separate flag that enables just EE-matching against the trust store?
In any case the issue is perhaps more subtle than reported in the PR summary.

I think the reason for the flag actually has to do with performance

I think the reason for the flag actually has to do with performance

The reason in the commit that introduced the flag does not sound like a performance optimisation:
commit 51e7a4378a78bb0870a2cdc5c524c230c929ebcb
Author: Dr. Stephen Henson <steve@openssl.org>
Date:   Thu Dec 13 18:14:46 2012 +0000

    New verify flag to return success if we have any certificate in the
    trusted store instead of the default which is to return an error if
    we can't build the complete chain.

commit 2dabd822366df7b2608b55d5ca5f31d5d484cbaf
Author: Dr. Stephen Henson <steve@openssl.org>
Date:   Fri Dec 21 18:31:32 2012 +0000

    Make partial chain checking work if we only have the EE certificate in
    the trust store.

This supports matching chains that wouldn't otherwise match, in particular also exact matches of the EE cert, which is the more compelling case. With intermediate certs the case is weaker (though Postfix needed this for DANE support until real DANE support landed in OpenSSL 1.1.0).
A careful reading of crypto/x509/x509_trs.c will show that the policy of anchoring to a self-signed root is in trust_compat() (which one can turn off with X509_TRUST_NO_SS_COMPAT), a backwards-compatibilty layer for absence of explicit policy.
Had explicit policy been socialised and required early enough, we could have more meaningful trust-stores now, with sensible limitations per-CA, ... (as in Firefox, MacOS, ...).  Instead the trust model is rather anaemic. But I am not sure that it should therefore degenerate all the way, precluding any hope of untrusted intermediates in the store.
Someone would also have to figure out how X509_TRUST_NO_SS_COMPAT and X509_V_FLAG_PARTIAL_CHAIN are expected to interact...

It would be really good that someone actually implemented a trust
store that can be used by OpenSSL that can have the same
restrictions as for instance Mozilla's trust store. As far as I
know, the X509_AUX structure should be general enough to implement
it. There is no reason that intermediate CAs or EE certs couldn't
also have that additioal data.
As far as I know, what's needed is:

Define and implement an OID / format for some of the restrictions
that we currently don't support
Implement the restrictions in the validation
Extend the tools to be able to write all the restrictions
Create tools to convert the trust settings from one of the root
stores.

But I guess this all belongs in some other issue.

According to the man page for X509_VERIFY_PARAM_set_flags:
The X509_V_FLAG_PARTIAL_CHAIN flag causes non-self-signed certificates in the trust store to be treated as trust anchors, in the same way as self-signed root CA certificates. This makes it possible to trust self-issued certificates as well as certificates issued by an intermediate CA without having to trust their ancestor root CA.

What we're talking about is what you can set with x509's -addtrust
and -addreject. That will turn it from a "CERTIFICATE" to a
"TRUSTED CERTIFICATE". Root stores contain certificates with this
trust information, but not in compatible way. OpenSSL is almost
always used with that trust information removed.

My bad, I thought this discussion was about X509_V_FLAG_PARTIAL_CHAIN.

... implemented a trust store that can be used by OpenSSL that can have the same restrictions as for instance Mozilla's trust store

That would require a way to swap-in different policies. The IETF and CA/B BR are different in the way they handle KU and EKU in the chain. If I recall correctly, The IETF is additive. If one CA in the chain has one key usage, and a second CA in the chain has another key usage, then the EE is evaluated using all key usages. That is, IETF policies treat them as additive or a union.
The CA/B BR is opposite. The key usage has to be present in all the CAs in the chain. If a key usage is miing in one CA in the chain, then the key usage is removed from the set.
So the OpenSSL API would need a way to swap-in the CA/B BR policy for handling chains. It would also need to document it since OpenSSL claims to be conforming to IETF standards, not the CA/B BR.

That would require a way to swap-in different policies. The IETF and CA/B BR are different in the way they handle KU and EKU in the chain. If I recall correctly, The IETF is additive. If one CA in the chain has one key usage, and a second CA in the chain has another key usage, then the EE is evaluated using all key usages. That is, IETF policies treat them as additive or a union.

I think for KU, each certificate needs to have the KU that it will
be used for, and the next certifiate can have others that are not
related for both. As far as I know there is no addition or removal
for KU.
For EKU, RFC5280 says: In general, this extension will appear only
in end entity certificates.
It does not really define what it means in CA certificates, so
implementations did different things. But now the BRs and at least
Mozilla's root policy do define what it means.

The discussion of EKU's inside certificates is not in scope here. In terms of trust-store management the EKU's are completely outside the signed certificate, they're trust-store metadata wrapped around the certificate.
Yes, there should be tools for managing CAfiles/CApaths with explicit trust settings for all trust anchors, and then X509_TRUST_NO_SS_COMPAT would make the policy stick. There does not need to be new code for consuming such stores, that code already exists...

There are certain constraints in root stores for which we currently
have no way to put them into an X509_AUX. For example, NSS has a way
to indicate that certificates issued after a certain date are no
longer trusted. We also have no way to blacklist certificates.
Information like that can be stored in the "other" part of X509_AUX,
if someone writes the code for it.
If people would start to use X509_AUX with what we already
support, that would be a good start.

Can we move the "should openssl have a root store" dicussion over to #13743?  And leave this issue to the relatively simple question being asked?  (And, I think, answered: "no")

While the "root store" discussion is perhaps indeed a separate topic in its own right, the two are not entirely orthogonal. If one enables the partial chain flag, one implicitly enables trust in intermediate certificates, for which there is no explicit trust policy.
An alternative, is to import these wrapped as "trusted certificates" with explicit trust EKUs (likely "any" in many cases).  So it is difficult to make a clean cut that separates this issue from the "root store" issue.
It ultimately all comes down to a gap in the design, with half-measures pulling in different directions.  My response was intended to make the participants aware of the full scope of the related issues.  Note that I did not explicitly say "no", to the proposal in this PR, though I'm certainly inclined to be cautious, because changing the default would complicate later introducing explicit trust. So my overall inclination is in the "no" direction.
I'd like to hear what others think, now that they've hopefully got a glimpse of the broader picture...

It would be really good that someone actually implemented a trust store that can be used by OpenSSL that can have the same restrictions as for instance Mozilla's trust store. As far as I know, the X509_AUX structure should be general enough to implement it. There is no reason that intermediate CAs or EE certs couldn't also have that additioal data.

Some years ago I suggested an IETF draft desired to store such limitations on the application-level basis (https://tools.ietf.org/html/draft-belyavskiy-certificate-limitation-policy-07), but it didn't get much interest.

@vdukhovni

If the intermediate is in the store, why not also include its issuers?

The issuer might eventually no longer be trusted, like when RSA 1024 certificates were phased out.
An issuer might have been used to cross sign the intermediate, but there might be multiple versions of a certificate - all having the same key material and same subject name - but one being cross signed, and another one with subject name == issuer name.

The partial chain flag is required because RFC 4158, Certification Path Building, requires a certificate path that is rooted to a Root CA. RFC 4158 does not allow an intermediate CA to be the root of trust. Root CA's are self signed and have "Issuer == Subject", "CA ==true, critical" and "spki==akid".

Thanks @noloader for this very interesting reference to RFC 4158!
Strangely, although this provides fundamental background on cert path construction, that RFC is hardly ever referenced.
Maybe this is because it is rather lengthy and does not provide an easy-to-follow recipe how to do the path construction.
Note that your above citation/conclusion is not correct.
https://tools.ietf.org/html/rfc4158#section-2.1 states:

A certification path is an ordered list of certificates starting with
a certificate that can be validated by one of the relying party's
trust anchors, and ending with the certificate to be validated.  (The
certificate to be validated is referred to as the "target
certificate" throughout this document.)  Though not required, as a
matter of convenience these trust anchors are typically stored in
trust anchor certificates.

What you call "Root CA" appears to correspond to what they define (in section 1.3) as

Trust Anchor Certificate:  A self-signed certificate for a trust
anchor that is used in certification path processing.

Such a Trust Anchor Certificate is self-signed, which implies self-issued and thus "Issuer == Subject".
Yet It does not imply "CA", so your presentation of acceptable paths wrongly excludes (self-signed) EE roots.
(BTW, self-signed certs should have an SKID, but usually don't have an AKID because it would be equal, as you wrote.)
The main error in your statement " requires a certificate path that is rooted to a Root CA" w.r.t. the question being discussed here is that actually there is no requirement in RFC 4148 (and neither in RFC 5280 etc.) that a trust anchor is given in the form of a self-signed cert!
A trust anchor may be given in the form of any cert, and and does not even need to be a certificate at all:

Trust Anchor: The combination of a trusted public key and the name of
the entity to which the corresponding private key belongs.

So also the conclusion "RFC 4158 does not allow an intermediate CA to be the root of trust" is wrong.
On the contrary, any public key and corresponding subject DN can be taken as a trust anchor.
In particular, any certificate can serve as a trust anchor - it just depends on which certs are considered trustworthy.

The partial chain flag is required because RFC 4158, Certification Path Building, requires a certificate path that is rooted to a Root CA. RFC 4158 does not allow an intermediate CA to be the root of trust. Root CA's are self signed and have "Issuer == Subject", "CA ==true, critical" and "spki==akid".

Thanks @noloader for this very interesting reference to RFC 4158!
Strangely, although this provides fundamental background on cert path construction, that RFC is hardly ever referenced.
Maybe this is because it is rather lengthy and does not provide an easy-to-follow recipe how to do the path construction.
Note that your above citation/conclusion is not correct.
https://tools.ietf.org/html/rfc4158#section-2.1 states:

A certification path is an ordered list of certificates starting with
a certificate that can be validated by one of the relying party's
trust anchors, and ending with the certificate to be validated.  (The
certificate to be validated is referred to as the "target
certificate" throughout this document.)  Though not required, as a
matter of convenience these trust anchors are typically stored in
trust anchor certificates.

What you call "Root CA" appears to correspond to what they define (in section 1.3) as

Trust Anchor Certificate:  A self-signed certificate for a trust
anchor that is used in certification path processing.

Such a Trust Anchor Certificate is self-signed, which implies self-issued and thus "Issuer == Subject".
Yet It does not imply "CA", so your presentation of acceptable paths wrongly excludes (self-signed) EE roots.
(BTW, self-signed certs should have an SKID, but usually don't have an AKID because it would be equal, as you wrote.)
The main error in your statement " requires a certificate path that is rooted to a Root CA" w.r.t. the question being discussed here is that actually there is no requirement in RFC 4148 (and neither in RFC 5280 etc.) that a trust anchor is given in the form of a self-signed cert!
A trust anchor may be given in the form of any cert, and and does not even need to be a certificate at all:

Trust Anchor: The combination of a trusted public key and the name of
the entity to which the corresponding private key belongs.

So also the conclusion "RFC 4158 does not allow an intermediate CA to be the root of trust" is wrong.
On the contrary, any public key and corresponding subject DN can be taken as a trust anchor.
In particular, any certificate can serve as a trust anchor - it just depends on which certs are considered trustworthy.

In other words, it's a matter of convention/policy (and nothing else) whether a cert is acceptable as a trust anchor and thus is allowed to terminate a chain as the acceptable outcome of chain building.
As @vdukhovni nicely pointed out, this issue is strongly related to the trust settings for certs
and that most people do not understand (or at least tend to neglect) those.
Part of the reason for this is that the OpenSSL documentation does not prominently describe them.
AFAICS, OpenSSL (so far) has/implements the policy that a cert is considered a trust anchor for the given (maybe default) purpose if and only if

it is part of the trust store (i.e., the list of trusted certs)
its does not have trust attributes rejecting the purpose
it has a trust attribute accepting the purpose
or it is self-signed (while there are obscure flags X509_TRUST_NO_SS_COMPAT and X509_TRUST_DO_SS_COMPAT)
or X509_V_FLAG_PARTIAL_CHAIN is set.

This explains the semantics of X509_V_FLAG_PARTIAL_CHAIN, right?
Next question: Should it become the default?
I tend to say yes because

this would bring consistency with the already existing exception for self-signed certs in the trust store,
namely that also non-self-signed certs in the trust store are by default trusted,
this is more intuitive to use since then everything in the trust store is by default trusted, and
this makes the OpenSSL behavior more consistent with other libraries

In other words, it's a matter of convention/policy (and nothing else) whether a cert is acceptable as a trust anchor and thus is allowed to terminate a chain as the acceptable outcome of chain building.
As @vdukhovni nicely pointed out, this issue is strongly related to the trust settings for certs
and that most people do not understand (or at least tend to neglect) those.
Part of the reason for this is that the OpenSSL documentation does not prominently describe them.
AFAICS, OpenSSL (so far) has/implements the policy that a cert is considered a trust anchor for the given (maybe default) purpose if and only if

it is part of the trust store (i.e., the list of trusted certs)
its does not have trust attributes rejecting the purpose
it has a trust attribute accepting the purpose
or it is self-signed (while there are obscure flags X509_TRUST_NO_SS_COMPAT and X509_TRUST_DO_SS_COMPAT)
or X509_V_FLAG_PARTIAL_CHAIN is set.

This explains the semantics of X509_V_FLAG_PARTIAL_CHAIN, right?
Next question: Should it become the default?
I tend to say yes because

this would bring consistency with the already existing exception for self-signed certs in the trust store,
namely that also non-self-signed certs in the trust store are by default trusted,
this is more intuitive to use since then everything in the trust store is by default trusted, and
this makes the OpenSSL behavior more consistent with other libraries

@DDvO,

Note that your above citation/conclusion is not correct...

Check out definitions from Section 1.3:

Trust Anchor Certificate:  A self-signed certificate for a trust
anchor that is used in certification path processing.

Your trust anchors must be self signed when path building.

@DDvO,

Note that your above citation/conclusion is not correct...

Check out definitions from Section 1.3:

Trust Anchor Certificate:  A self-signed certificate for a trust
anchor that is used in certification path processing.

Your trust anchors must be self signed when path building.

AFAICS, OpenSSL (so far) has/implements the policy that a cert is considered a trust anchor for the given (maybe default) purpose if and only if

it is part of the trust store (i.e., the list of trusted certs)
its does not have trust attributes rejecting the purpose
it has a trust attribute accepting the purpose
or it is self-signed (while there are obscure flags X509_TRUST_NO_SS_COMPAT and X509_TRUST_DO_SS_COMPAT)
or X509_V_FLAG_PARTIAL_CHAIN is set.

This explains the semantics of X509_V_FLAG_PARTIAL_CHAIN, right?

Close enough.  EE matching is special, in that the certs have to compare equal, not just have the same key, ...

Next question: Should it become the default?
I tend to say yes because

this would bring consistency with the already existing exception for self-signed certs in the trust store,
namely that also non-self-signed certs in the trust store are by default trusted,
this is more intuitive to use since then everything in the trust store is by default trusted, and
this makes the OpenSSL behavior more consistent with other libraries

I don't know about "other libraries", but in terms of OpenSSL, my concern is that doing so takes further away from some day ideally having actual trust settings on all trust anchors.  I'd rather see users who want trusted intermediates in the store wrap them accordingly, so that they actually become trust anchors explicitly.  That said, I don't personally have a use-case that breaks if partial chain becomes the default, but some applications may behave in unexpected ways if chain building starts producing unexpectedly shorter chains, not anchored at a "root CA".  Caution is advisable.

AFAICS, OpenSSL (so far) has/implements the policy that a cert is considered a trust anchor for the given (maybe default) purpose if and only if

it is part of the trust store (i.e., the list of trusted certs)
its does not have trust attributes rejecting the purpose
it has a trust attribute accepting the purpose
or it is self-signed (while there are obscure flags X509_TRUST_NO_SS_COMPAT and X509_TRUST_DO_SS_COMPAT)
or X509_V_FLAG_PARTIAL_CHAIN is set.

This explains the semantics of X509_V_FLAG_PARTIAL_CHAIN, right?

Close enough.  EE matching is special, in that the certs have to compare equal, not just have the same key, ...

Next question: Should it become the default?
I tend to say yes because

this would bring consistency with the already existing exception for self-signed certs in the trust store,
namely that also non-self-signed certs in the trust store are by default trusted,
this is more intuitive to use since then everything in the trust store is by default trusted, and
this makes the OpenSSL behavior more consistent with other libraries

I don't know about "other libraries", but in terms of OpenSSL, my concern is that doing so takes further away from some day ideally having actual trust settings on all trust anchors.  I'd rather see users who want trusted intermediates in the store wrap them accordingly, so that they actually become trust anchors explicitly.  That said, I don't personally have a use-case that breaks if partial chain becomes the default, but some applications may behave in unexpected ways if chain building starts producing unexpectedly shorter chains, not anchored at a "root CA".  Caution is advisable.

@DDvO,

Note that your above citation/conclusion is not correct...

Check out definitions from Section 1.3:

Trust Anchor Certificate:  A self-signed certificate for a trust
anchor that is used in certification path processing.

Your trust anchors must be self signed when path building.
That's the only problem I have with this RFC. They do not allow us to build a path to an arbitrary CA certificate.

@noloader, no, you are confusing this definition with a necessary condition (for serving as a trust anchor).
It is not necessary that a TA is self-signed!
Otherwise, the X509_V_FLAG_PARTIAL_CHAIN would be illegal BTW :wink:
See also one of the sentences I quoted above from section 2.1:

Though not required, as a
matter of convenience these trust anchors are typically stored in
trust anchor certificates.

As I wrote, and also stated in that RFC and several others,
a TA can be any public key and the related subject DN.

@DDvO,

Note that your above citation/conclusion is not correct...

Check out definitions from Section 1.3:

Trust Anchor Certificate:  A self-signed certificate for a trust
anchor that is used in certification path processing.

Your trust anchors must be self signed when path building.
That's the only problem I have with this RFC. They do not allow us to build a path to an arbitrary CA certificate.

@noloader, no, you are confusing this definition with a necessary condition (for serving as a trust anchor).
It is not necessary that a TA is self-signed!
Otherwise, the X509_V_FLAG_PARTIAL_CHAIN would be illegal BTW :wink:
See also one of the sentences I quoted above from section 2.1:

Though not required, as a
matter of convenience these trust anchors are typically stored in
trust anchor certificates.

As I wrote, and also stated in that RFC and several others,
a TA can be any public key and the related subject DN.

As I wrote, and also stated in that RFC and several others,
a TA can be any public key and the related subject DN.

Good luck finding a something PKIX that only uses public keys.

As I wrote, and also stated in that RFC and several others,
a TA can be any public key and the related subject DN.

Good luck finding a something PKIX that only uses public keys.

As I wrote, and also stated in that RFC and several others,
a TA can be any public key and the related subject DN.

Good luck finding a something PKIX that only uses public keys.

This just states the full generality.
Of course, in practice TAs do have the form of a certificate, but even then - contrary to what PKI novices may think - being a self-signed CA cert is neither necessary nor sufficient for being a TA!
See also the excellent statements by @vdukhovni above on other conditions for a cert to serve as a TA.

As I wrote, and also stated in that RFC and several others,
a TA can be any public key and the related subject DN.

Good luck finding a something PKIX that only uses public keys.

This just states the full generality.
Of course, in practice TAs do have the form of a certificate, but even then - contrary to what PKI novices may think - being a self-signed CA cert is neither necessary nor sufficient for being a TA!
See also the excellent statements by @vdukhovni above on other conditions for a cert to serve as a TA.

AFAICS, OpenSSL (so far) has/implements the policy that a cert is considered a trust anchor for the given (maybe default) purpose if and only if

it is part of the trust store (i.e., the list of trusted certs)
its does not have trust attributes rejecting the purpose
it has a trust attribute accepting the purpose
or it is self-signed (while there are obscure flags X509_TRUST_NO_SS_COMPAT and X509_TRUST_DO_SS_COMPAT)
or X509_V_FLAG_PARTIAL_CHAIN is set.

This explains the semantics of X509_V_FLAG_PARTIAL_CHAIN, right?

Close enough. EE matching is special, in that the certs have to compare equal, not just have the same key, ...

This is essentially what I meant by "is part of the trust store", but then I did not get the CA cert case right.
So how about this version?
A certificate is considered a trust anchor for the given purpose (or, by default, any purpose) of the target certificate if and only if

it is an EE certificate that is in the list of trusted certificates
or it is a CA cert with its subject, subject key identifier (if present), and key algorithm matching an entry in the trust store
it does not have trust attributes rejecting the purpose
it has a trust attribute accepting the purpose
or it is self-signed
or X509_V_FLAG_PARTIAL_CHAIN is set.

If this is (sufficiently) correct now, I'd suggest placing this in doc/man1/openssl-verification-options.pod.

AFAICS, OpenSSL (so far) has/implements the policy that a cert is considered a trust anchor for the given (maybe default) purpose if and only if

it is part of the trust store (i.e., the list of trusted certs)
its does not have trust attributes rejecting the purpose
it has a trust attribute accepting the purpose
or it is self-signed (while there are obscure flags X509_TRUST_NO_SS_COMPAT and X509_TRUST_DO_SS_COMPAT)
or X509_V_FLAG_PARTIAL_CHAIN is set.

This explains the semantics of X509_V_FLAG_PARTIAL_CHAIN, right?

Close enough. EE matching is special, in that the certs have to compare equal, not just have the same key, ...

This is essentially what I meant by "is part of the trust store", but then I did not get the CA cert case right.
So how about this version?
A certificate is considered a trust anchor for the given purpose (or, by default, any purpose) of the target certificate if and only if

it is an EE certificate that is in the list of trusted certificates
or it is a CA cert with its subject, subject key identifier (if present), and key algorithm matching an entry in the trust store
it does not have trust attributes rejecting the purpose
it has a trust attribute accepting the purpose
or it is self-signed
or X509_V_FLAG_PARTIAL_CHAIN is set.

If this is (sufficiently) correct now, I'd suggest placing this in doc/man1/openssl-verification-options.pod.

AFAICS, OpenSSL (so far) has/implements the policy that a cert is considered a trust anchor for the given (maybe default) purpose if and only if

it is part of the trust store (i.e., the list of trusted certs)
its does not have trust attributes rejecting the purpose
it has a trust attribute accepting the purpose
or it is self-signed (while there are obscure flags X509_TRUST_NO_SS_COMPAT and X509_TRUST_DO_SS_COMPAT)
or X509_V_FLAG_PARTIAL_CHAIN is set.

This explains the semantics of X509_V_FLAG_PARTIAL_CHAIN, right?

Close enough. EE matching is special, in that the certs have to compare equal, not just have the same key, ...

This is essentially what I meant by "is part of the trust store", but then I did not get the CA cert case right.
So how about this version?
A certificate is considered a trust anchor for the given purpose (or, by default, any purpose) of the target certificate if and only if

it is an EE certificate that is in the list of trusted certificates
or it is a CA cert with its subject, subject key identifier (if present), and key algorithm matching an entry in the trust store
it does not have trust attributes rejecting the purpose
it has a trust attribute accepting the purpose
or it is self-signed
or X509_V_FLAG_PARTIAL_CHAIN is set.

If this is (sufficiently) correct now, I'd suggest placing this in doc/man1/openssl-verification-options.pod.

AFAICS, OpenSSL (so far) has/implements the policy that a cert is considered a trust anchor for the given (maybe default) purpose if and only if

it is part of the trust store (i.e., the list of trusted certs)
its does not have trust attributes rejecting the purpose
it has a trust attribute accepting the purpose
or it is self-signed (while there are obscure flags X509_TRUST_NO_SS_COMPAT and X509_TRUST_DO_SS_COMPAT)
or X509_V_FLAG_PARTIAL_CHAIN is set.

This explains the semantics of X509_V_FLAG_PARTIAL_CHAIN, right?

Close enough. EE matching is special, in that the certs have to compare equal, not just have the same key, ...

This is essentially what I meant by "is part of the trust store", but then I did not get the CA cert case right.
So how about this version?
A certificate is considered a trust anchor for the given purpose (or, by default, any purpose) of the target certificate if and only if

it is an EE certificate that is in the list of trusted certificates
or it is a CA cert with its subject, subject key identifier (if present), and key algorithm matching an entry in the trust store
it does not have trust attributes rejecting the purpose
it has a trust attribute accepting the purpose
or it is self-signed
or X509_V_FLAG_PARTIAL_CHAIN is set.

If this is (sufficiently) correct now, I'd suggest placing this in doc/man1/openssl-verification-options.pod.

Next question: Should it become the default?
I tend to say yes because

this would bring consistency with the already existing exception for self-signed certs in the trust store,
namely that also non-self-signed certs in the trust store are by default trusted,
this is more intuitive to use since then everything in the trust store is by default trusted, and
this makes the OpenSSL behavior more consistent with other libraries

I don't know about "other libraries",

What I meant is in particular gnuTLS mentioned by @noloader
and what @t8m wrote in the original post above:

any other major TLS library behaves like this by default not requiring any such extra verification flags.

@vdukhovni wrote:

but in terms of OpenSSL, my concern is that doing so takes further away from some day ideally having actual trust settings on all trust anchors. I'd rather see users who want trusted intermediates in the store wrap them accordingly, so that they actually become trust anchors explicitly.

I see your point. Yet the vast majority of users has no clue about explicit (dis-)trust settings,
and even people like me took quite a while to find out about these desirable "advanced" features.
Part of the reason is that the OpenSSL documentation so far failed to mention them clearly enough.

That said, I don't personally have a use-case that breaks if partial chain becomes the default,

Can anyone else bring up such a use case?

Next question: Should it become the default?
I tend to say yes because

this would bring consistency with the already existing exception for self-signed certs in the trust store,
namely that also non-self-signed certs in the trust store are by default trusted,
this is more intuitive to use since then everything in the trust store is by default trusted, and
this makes the OpenSSL behavior more consistent with other libraries

I don't know about "other libraries",

What I meant is in particular gnuTLS mentioned by @noloader
and what @t8m wrote in the original post above:

any other major TLS library behaves like this by default not requiring any such extra verification flags.

@vdukhovni wrote:

but in terms of OpenSSL, my concern is that doing so takes further away from some day ideally having actual trust settings on all trust anchors. I'd rather see users who want trusted intermediates in the store wrap them accordingly, so that they actually become trust anchors explicitly.

I see your point. Yet the vast majority of users has no clue about explicit (dis-)trust settings,
and even people like me took quite a while to find out about these desirable "advanced" features.
Part of the reason is that the OpenSSL documentation so far failed to mention them clearly enough.

That said, I don't personally have a use-case that breaks if partial chain becomes the default,

Can anyone else bring up such a use case?

@vdukhovni wrote:

but some applications may behave in unexpected ways if chain building starts producing unexpectedly shorter chains, not anchored at a "root CA". Caution is advisable.

That's true, and this is also the main concern of @richsalz here:

Please be very careful, and do not change the verification behavior to be "more correct" (quotes on purpose).

Many organizations depend on the current, documented, behavior.

Well, the documentation so far is pretty poor (BTW, see also my PR #13735 which aims at partially alleviating this),
so organizations so far have little chance to depend on the documented behavior.
It would be more realistic to say that they got used to the implemented behavior, be it for good or bad.

And leave this issue to the relatively simple question being asked? (And, I think, answered: "no")

It should have become apparent that the issue is not as simple as one might hope.

@vdukhovni wrote:

but some applications may behave in unexpected ways if chain building starts producing unexpectedly shorter chains, not anchored at a "root CA". Caution is advisable.

That's true, and this is also the main concern of @richsalz here:

Please be very careful, and do not change the verification behavior to be "more correct" (quotes on purpose).

Many organizations depend on the current, documented, behavior.

Well, the documentation so far is pretty poor (BTW, see also my PR #13735 which aims at partially alleviating this),
so organizations so far have little chance to depend on the documented behavior.
It would be more realistic to say that they got used to the implemented behavior, be it for good or bad.

And leave this issue to the relatively simple question being asked? (And, I think, answered: "no")

It should have become apparent that the issue is not as simple as one might hope.

One thing to keep in mind is that at least for DANE-TA(2) and PKIX-TA(0) the chain needs to be build at least as far as an actual trust anchor matching the TLSA record, even if some intermediate CA is found in the store, the chain building needs to continue.
There should be logic in place to make sure that partial chain does not break this, and it should be preserved (or fixed if deficient).  Otherwise, as you may have noticed, I am maintaining a cautiously neutral stance. The proposal has some merit, but the risks are not completely understood, and I'd rather see movement towards explicit trust anchors instead of more implicit ones.

One thing to keep in mind is that at least for DANE-TA(2) and PKIX-TA(0) the chain needs to be build at least as far as an actual trust anchor matching the TLSA record, even if some intermediate CA is found in the store, the chain building needs to continue.
There should be logic in place to make sure that partial chain does not break this, and it should be preserved (or fixed if deficient).  Otherwise, as you may have noticed, I am maintaining a cautiously neutral stance. The proposal has some merit, but the risks are not completely understood, and I'd rather see movement towards explicit trust anchors instead of more implicit ones.

Ah, yes, the whole DANE topic is a further complication of the matter, also what concerns the readability of the code.
Since apparently you implemented this, can you take care of providing some high-level documentation for it in
openssl-verification-options.pod?
So far, DANE is non mentioned at all in high-level documentation, except for some options of the s_client app.
And is it fair to say that my above description attempt now correctly sums up the trust anchor topic as long as DANE is not enabled, such that I can bring it into #13735 with this (non-DANE) restriction being explicitly made?

Ah, yes, the whole DANE topic is a further complication of the matter, also what concerns the readability of the code.
Since apparently you implemented this, can you take care of providing some high-level documentation for it in
openssl-verification-options.pod?
So far, DANE is non mentioned at all in high-level documentation, except for some options of the s_client app.
And is it fair to say that my above description attempt now correctly sums up the trust anchor topic as long as DANE is not enabled, such that I can bring it into #13735 with this (non-DANE) restriction being explicitly made?

I've never seen nor heard of openssl-verification-options.pod.  Perhaps it is something new, that postdates the DANE implementation.
DANE is documented in: s_client(1), SSL_CTX_dane_enable(3), SSL_set1_host(3), X509_VERIFY_PARAM_set_flags(3) and X509_check_host(3).

I've never seen nor heard of openssl-verification-options.pod.  Perhaps it is something new, that postdates the DANE implementation.
DANE is documented in: s_client(1), SSL_CTX_dane_enable(3), SSL_set1_host(3), X509_VERIFY_PARAM_set_flags(3) and X509_check_host(3).

Seems you created this file on 2020-10-04, definitely after DANE.

Seems you created this file on 2020-10-04, definitely after DANE.

As far as readability, by the way, the chain construction code in x509_vfy.c was rewritten as part of the DANE implementation in 1.1.0, and is now substantially more readable/maintainable than the prior state in 1.0.2.

As far as readability, by the way, the chain construction code in x509_vfy.c was rewritten as part of the DANE implementation in 1.1.0, and is now substantially more readable/maintainable than the prior state in 1.0.2.

As for DANE in verify options, it is not applicable there, because DANE is only supported for TLS (based on, surprise, DNS TLSA records), so there are no generic DANE verification options for smime, cms, ...  And in TLS DANE is a client-side feature, so its documentation is in s_client(1) and SSL_CTX_dane_enable(3), ... where it belongs.

As for DANE in verify options, it is not applicable there, because DANE is only supported for TLS (based on, surprise, DNS TLSA records), so there are no generic DANE verification options for smime, cms, ...  And in TLS DANE is a client-side feature, so its documentation is in s_client(1) and SSL_CTX_dane_enable(3), ... where it belongs.

@ddvo, you're new to the project, so it's understandable you are getting caught up on the phrase "documented behavior."  For years OpenSSL has had bad documentation of many things, and when long-time folks talk about "documented behavior" they can mean "implemented" because that's the only thing that was available.   It's not fair to document different behavior and say it's fixing a bug, as opposed to documenting what the code does.  (And yes, the DANE stuff is well-documented.)
At this point, nobody else seems to be in favor of #13748 and your comments are based on some errors.

@ddvo, you're new to the project, so it's understandable you are getting caught up on the phrase "documented behavior."  For years OpenSSL has had bad documentation of many things, and when long-time folks talk about "documented behavior" they can mean "implemented" because that's the only thing that was available.   It's not fair to document different behavior and say it's fixing a bug, as opposed to documenting what the code does.  (And yes, the DANE stuff is well-documented.)
At this point, nobody else seems to be in favor of #13748 and your comments are based on some errors.

Seems you created this file on 2020-10-04, definitely after DANE.

Yes. I've concentrated there (and extended) the more general documentation of cert path construction and verification,
because the documentation about the topic in other places was pretty sparse and partly overlapping.
Whether the doc the should include the TLS (and thus DANE) topic in my view is a matter of taste.

Seems you created this file on 2020-10-04, definitely after DANE.

Yes. I've concentrated there (and extended) the more general documentation of cert path construction and verification,
because the documentation about the topic in other places was pretty sparse and partly overlapping.
Whether the doc the should include the TLS (and thus DANE) topic in my view is a matter of taste.

Whether the doc the should include the TLS (and thus DANE) topic in my view is a matter of taste.

The stuff in man1 talks about the applications.  Arguably there coujld be an man3/ssl-verify that points to common things in the man1 page, and also includes pointers to the DANE info.
I don't think it's a matter of taste, I think it's more clear then that.

Whether the doc the should include the TLS (and thus DANE) topic in my view is a matter of taste.

The stuff in man1 talks about the applications.  Arguably there coujld be an man3/ssl-verify that points to common things in the man1 page, and also includes pointers to the DANE info.
I don't think it's a matter of taste, I think it's more clear then that.

DANE is documented in: s_client(1), SSL_CTX_dane_enable(3), SSL_set1_host(3), X509_VERIFY_PARAM_set_flags(3) and X509_check_host(3).

Does that documentation include a high-level description of the DANE-oriented cert path construction and verification?
I'm just asking this in the (to me hypothetical) interest of DANE users; I don't have the time to dive into that topic myself.

DANE is documented in: s_client(1), SSL_CTX_dane_enable(3), SSL_set1_host(3), X509_VERIFY_PARAM_set_flags(3) and X509_check_host(3).

Does that documentation include a high-level description of the DANE-oriented cert path construction and verification?
I'm just asking this in the (to me hypothetical) interest of DANE users; I don't have the time to dive into that topic myself.

Does that documentation include a high-level description of the DANE-oriented cert path construction and verification?
I'm just asking this in the (to me hypothetical) interest of DANE users; I don't have the time to dive into that topic myself.

OpenSSL developers are not the audience of the s_client(1) manpage.  It describes the user-interface sufficiently for a user to understand the relevant options:
       -dane_tlsa_domain domain
           Enable RFC6698/RFC7671 DANE TLSA authentication and specify the
           TLSA base domain which becomes the default SNI hint and the primary
           reference identifier for hostname checks.  This must be used in
           combination with at least one instance of the -dane_tlsa_rrdata
           option below.

           When DANE authentication succeeds, the diagnostic output will
           include the lowest (closest to 0) depth at which a TLSA record
           authenticated a chain certificate.  When that TLSA record is a "2 1
           0" trust anchor public key that signed (rather than matched) the
           top-most certificate of the chain, the result is reported as "TA
           public key verified".  Otherwise, either the TLSA record "matched
           TA certificate" at a positive depth or else "matched EE
           certificate" at depth 0.

       -dane_tlsa_rrdata rrdata
           Use one or more times to specify the RRDATA fields of the DANE TLSA
           RRset associated with the target service.  The rrdata value is
           specified in "presentation form", that is four whitespace separated
           fields that specify the usage, selector, matching type and
           associated data, with the last of these encoded in hexadecimal.
           Optional whitespace is ignored in the associated data field.  For
           example:

             $ openssl s_client -brief -starttls smtp \
               -connect smtp.example.com:25 \
               -dane_tlsa_domain smtp.example.com \
               -dane_tlsa_rrdata "2 1 1
                 B111DD8A1C2091A89BD4FD60C57F0716CCE50FEEFF8137CDBEE0326E 02CF362B" \
               -dane_tlsa_rrdata "2 1 1
                 60B87575447DCBA2A36B7D11AC09FB24A9DB406FEE12D2CC90180517 616E8A18"
             ...
             Verification: OK
             Verified peername: smtp.example.com
             DANE TLSA 2 1 1 ...ee12d2cc90180517616e8a18 matched TA certificate at depth 1
             ...

       -dane_ee_no_namechecks
           This disables server name checks when authenticating via DANE-EE(3)
           TLSA records.  For some applications, primarily web browsers, it is
           not safe to disable name checks due to "unknown key share" attacks,
           in which a malicious server can convince a client that a connection
           to a victim server is instead a secure connection to the malicious
           server.  The malicious server may then be able to violate cross-
           origin scripting restrictions.  Thus, despite the text of RFC7671,
           name checks are by default enabled for DANE-EE(3) TLSA records, and
           can be disabled in applications where it is safe to do so.  In
           particular, SMTP and XMPP clients should set this option as SRV and
           MX records already make it possible for a remote domain to redirect
           client connections to any server of its choice, and in any case
           SMTP and XMPP clients do not execute scripts downloaded from remote
           servers.

The SSL_CTX_dane_enable(3) page similarly addresses the needs of application developers who want to enable certificate-verification a-la-DANE. It is not an internal implementation guide.

Does that documentation include a high-level description of the DANE-oriented cert path construction and verification?
I'm just asking this in the (to me hypothetical) interest of DANE users; I don't have the time to dive into that topic myself.

OpenSSL developers are not the audience of the s_client(1) manpage.  It describes the user-interface sufficiently for a user to understand the relevant options:
       -dane_tlsa_domain domain
           Enable RFC6698/RFC7671 DANE TLSA authentication and specify the
           TLSA base domain which becomes the default SNI hint and the primary
           reference identifier for hostname checks.  This must be used in
           combination with at least one instance of the -dane_tlsa_rrdata
           option below.

           When DANE authentication succeeds, the diagnostic output will
           include the lowest (closest to 0) depth at which a TLSA record
           authenticated a chain certificate.  When that TLSA record is a "2 1
           0" trust anchor public key that signed (rather than matched) the
           top-most certificate of the chain, the result is reported as "TA
           public key verified".  Otherwise, either the TLSA record "matched
           TA certificate" at a positive depth or else "matched EE
           certificate" at depth 0.

       -dane_tlsa_rrdata rrdata
           Use one or more times to specify the RRDATA fields of the DANE TLSA
           RRset associated with the target service.  The rrdata value is
           specified in "presentation form", that is four whitespace separated
           fields that specify the usage, selector, matching type and
           associated data, with the last of these encoded in hexadecimal.
           Optional whitespace is ignored in the associated data field.  For
           example:

             $ openssl s_client -brief -starttls smtp \
               -connect smtp.example.com:25 \
               -dane_tlsa_domain smtp.example.com \
               -dane_tlsa_rrdata "2 1 1
                 B111DD8A1C2091A89BD4FD60C57F0716CCE50FEEFF8137CDBEE0326E 02CF362B" \
               -dane_tlsa_rrdata "2 1 1
                 60B87575447DCBA2A36B7D11AC09FB24A9DB406FEE12D2CC90180517 616E8A18"
             ...
             Verification: OK
             Verified peername: smtp.example.com
             DANE TLSA 2 1 1 ...ee12d2cc90180517616e8a18 matched TA certificate at depth 1
             ...

       -dane_ee_no_namechecks
           This disables server name checks when authenticating via DANE-EE(3)
           TLSA records.  For some applications, primarily web browsers, it is
           not safe to disable name checks due to "unknown key share" attacks,
           in which a malicious server can convince a client that a connection
           to a victim server is instead a secure connection to the malicious
           server.  The malicious server may then be able to violate cross-
           origin scripting restrictions.  Thus, despite the text of RFC7671,
           name checks are by default enabled for DANE-EE(3) TLSA records, and
           can be disabled in applications where it is safe to do so.  In
           particular, SMTP and XMPP clients should set this option as SRV and
           MX records already make it possible for a remote domain to redirect
           client connections to any server of its choice, and in any case
           SMTP and XMPP clients do not execute scripts downloaded from remote
           servers.

The SSL_CTX_dane_enable(3) page similarly addresses the needs of application developers who want to enable certificate-verification a-la-DANE. It is not an internal implementation guide.

@DDvO, you're new to the project, so it's understandable you are getting caught up on the phrase "documented behavior." For years OpenSSL has had bad documentation of many things, and when long-time folks talk about "documented behavior" they can mean "implemented" because that's the only thing that was available. It's not fair to document different behavior and say it's fixing a bug, as opposed to documenting what the code does. (And yes, the DANE stuff is well-documented.)
At this point, nobody else seems to be in favor of #13748 and your comments are based on some errors.

I thinks this somewhat overstates the strength of my objections. I am not strongly against the proposal, rather I am highlighting the potential issues, leaving the outcome open to further discussion...

@DDvO, you're new to the project, so it's understandable you are getting caught up on the phrase "documented behavior." For years OpenSSL has had bad documentation of many things, and when long-time folks talk about "documented behavior" they can mean "implemented" because that's the only thing that was available. It's not fair to document different behavior and say it's fixing a bug, as opposed to documenting what the code does. (And yes, the DANE stuff is well-documented.)
At this point, nobody else seems to be in favor of #13748 and your comments are based on some errors.

I thinks this somewhat overstates the strength of my objections. I am not strongly against the proposal, rather I am highlighting the potential issues, leaving the outcome open to further discussion...

It's not fair to document different behavior and say it's fixing a bug, as opposed to documenting what the code does. (And yes, the DANE stuff is well-documented.)
At this point, nobody else seems to be in favor of #13748 and your comments are based on some errors.

@richsalz, you are confusing things.
One topic is extending the documentation of existing(!) behavior, which is the (only) topic of #13735 and the part of the topic in this discussion that I am currently focusing on.
A related but different topic is extending the completeness of the chain construction (and consequently its documentation).
Note that #13748 started off as a bug fix of #13739, and then I noticed and tried out interesting potential to extend and optimize the chain building. You objected to those changes right away, apparently without having a close look at them, and although I explicitly asked you to state concretely what you fear could go wrong, you only gave very general remarks, which are not wrong but pretty superficial.
I agree that now, very close to the v3.0 release, is not a good point in time to make changes (except for clear bug fixes) to the non-trivial chain construction and verification stuff, and so I'll minimize #13748 and defer any more general improvements to post-3.0, also because this has already taken me much more of my personal time than I should allow for it.

It's not fair to document different behavior and say it's fixing a bug, as opposed to documenting what the code does. (And yes, the DANE stuff is well-documented.)
At this point, nobody else seems to be in favor of #13748 and your comments are based on some errors.

@richsalz, you are confusing things.
One topic is extending the documentation of existing(!) behavior, which is the (only) topic of #13735 and the part of the topic in this discussion that I am currently focusing on.
A related but different topic is extending the completeness of the chain construction (and consequently its documentation).
Note that #13748 started off as a bug fix of #13739, and then I noticed and tried out interesting potential to extend and optimize the chain building. You objected to those changes right away, apparently without having a close look at them, and although I explicitly asked you to state concretely what you fear could go wrong, you only gave very general remarks, which are not wrong but pretty superficial.
I agree that now, very close to the v3.0 release, is not a good point in time to make changes (except for clear bug fixes) to the non-trivial chain construction and verification stuff, and so I'll minimize #13748 and defer any more general improvements to post-3.0, also because this has already taken me much more of my personal time than I should allow for it.

DANE is documented in: s_client(1), SSL_CTX_dane_enable(3), SSL_set1_host(3), X509_VERIFY_PARAM_set_flags(3) and X509_check_host(3).

Thanks @vdukhovni for these pointers and the further comments you gave on part of them.
So I've extended the documentation update #13735 to include them.

DANE is documented in: s_client(1), SSL_CTX_dane_enable(3), SSL_set1_host(3), X509_VERIFY_PARAM_set_flags(3) and X509_check_host(3).

Thanks @vdukhovni for these pointers and the further comments you gave on part of them.
So I've extended the documentation update #13735 to include them.

I don't understand why there is a need for this especially because openssl s_client does fail is the entire chain is not in the trust store:
root@deblndw013x2j:/usr/ports-overlays/ldadw-custom
# openssl s_client -connect deblndw011x.ad001.siemens.net:443 -CApath /etc/ssl/certs
CONNECTED(00000003)
depth=1 C = DE, ST = Bayern, L = Muenchen, O = Siemens, serialNumber = ZZZZZZE7, CN = Siemens Issuing CA Intranet Server 2022
verify error:num=2:unable to get issuer certificate
issuer= C = DE, ST = Bayern, L = Muenchen, O = Siemens, serialNumber = ZZZZZZA1, OU = Siemens Trust Center, CN = Siemens Root CA V3.0 2016
verify return:1
depth=0 C = DE, O = Siemens, OU = LDA IT IN, CN = deblndw011x.ad001.siemens.net
issuer= C = DE, ST = Bayern, L = Muenchen, O = Siemens, serialNumber = ZZZZZZE7, CN = Siemens Issuing CA Intranet Server 2022
verify return:1
---
Certificate chain
 0 s:C = DE, O = Siemens, OU = LDA IT IN, CN = deblndw011x.ad001.siemens.net
   i:C = DE, ST = Bayern, L = Muenchen, O = Siemens, serialNumber = ZZZZZZE7, CN = Siemens Issuing CA Intranet Server 2022
 1 s:C = DE, ST = Bayern, L = Muenchen, O = Siemens, serialNumber = ZZZZZZE7, CN = Siemens Issuing CA Intranet Server 2022
   i:C = DE, ST = Bayern, L = Muenchen, O = Siemens, serialNumber = ZZZZZZA1, OU = Siemens Trust Center, CN = Siemens Root CA V3.0 2016
 2 s:C = DE, ST = Bayern, L = Muenchen, O = Siemens, serialNumber = ZZZZZZA1, OU = Siemens Trust Center, CN = Siemens Root CA V3.0 2016
   i:C = DE, ST = Bayern, L = Muenchen, O = Siemens, serialNumber = ZZZZZZA1, OU = Siemens Trust Center, CN = Siemens Root CA V3.0 2016
---
Server certificate

Trust store:
# certctl list | grep Siemens
0c6840e5.0      Siemens Issuing CA MSA Impersonalized Entities 2023
0e436b80.0      Siemens Issuing CA Class OneAD 04
0e91b229.0      Siemens Issuing CA Class OneAD 14
17c10339.0      Siemens Issuing CA Class OneAD 05
18bd5750.0      Siemens Issuing CA EE Network Smartcard Auth 2021
1a6b61c8.0      Siemens Intermediate CA 2021
1c683ff3.0      Siemens Issuing CA Class OneAD 13
2c3a2e0f.0      Siemens Issuing CA EE Auth 2021
31e09dd6.0      Siemens Issuing CA Class OneAD 11
3210d285.0      Siemens Issuing CA Class OneAD 03
36971e57.0      Siemens Issuing CA EE Network Smartcard Auth 2023
3e1ec334.0      Siemens Issuing CA Intranet Code Signing 2020
4158715d.0      Siemens Issuing CA EE Enc 2023
55683be0.0      Siemens Issuing CA EE Auth 2020
5eeeed34.0      Siemens Issuing CA Intranet Code Signing 2023
6b9a4307.0      Siemens Issuing CA EE Auth 2023
7b1c186e.0      Siemens Issuing CA Class OneAD 06
81841955.0      Siemens Issuing CA EE Network Smartcard Auth 2020
8aac0ad6.0      Siemens Issuing CA EE Enc 2020
8af2d467.0      Siemens OneAD Root CA
9245e478.0      Siemens Issuing CA Class OneAD 02
940ae196.0      Siemens Issuing CA Intranet Server 2022
96f028d4.0      Siemens Issuing CA EE Enc 2021
bc2924b7.0      Siemens Issuing CA Class OneAD 01
bd411d1f.0      Siemens Issuing CA Class OneAD 00
c2cf79c6.0      Siemens Issuing CA Class OneAD 12
f103366b.0      Siemens Issuing CA Class OneAD 07

"Siemens Root CA V3.0 2016" is missing which is my fault. From a user's PoV this is more than suprising and can neither disabled at compile time nor at curl(1) runtime.
Several other tools fail here as well: fetch(1), svn(1).

You should write more clearly what you see as a problem here.
Your trust store should/must not contain intermediate CA certificates, but  just trusted (root) CA certs.
Hope you understand the difference between root CA certs and intermediate CA certs?
The latter are not trusted but are needed for chain building.
Moreover, they are usually provided by the peer - so also from this perspective it makes little sense to put them in the trust store.

You should write more clearly what you see as a problem here.

My expectation that default behavior is to verify the entire chain and not stop when the first intermediate is validated.

Your trust store should/must not contain intermediate CA certificates, but just trusted (root) CA certs.

Why? This isn't clear.

Hope you understand the difference between root CA certs and intermediate CA certs? The latter are not trusted but are needed for chain building. Moreover, they are usually provided by the peer - so also from this perspective it makes little sense to put them in the trust store.

Yes, I do understand why where should those be provided by the peer? None of the certs signed by our CA contain the intermeidate(s) as DER, the subject only of course.

@michael-o,

I don't understand why there is a need for this especially because openssl s_client does fail is the entire chain is not in the trust store:

RFC 4158, Certification Path Building, says path building happens between the end-entity certificate to a "trust anchor."  Eventually, the "trust anchor certificate" is defined thusly: "a self-signed certificate for a trust anchor that is used in certification path processing." Note that the self-signed requirement makes a trust anchor a traditional Root CA certificate.
If you want to root your trust in a non-root (non-self-signed) certificate, then you need to use X509_V_FLAG_PARTIAL_CHAIN.
Personally, I've never liked the fact the RFC refers to "trust anchor certificate" and defines them in terms of being self-signed. It is up to the relying party (RP) to decide what they want to trust. If a RP wishes to root trust in an intermediate CA certificate, like Let's Encrypt E1 or E2, then they should be able to without trusting the certifying CA with the self-signed certificate.
And while its not obvious... when you trust the Root CA, you bring in the entire PKI tree, and not just the bits you need from a branch. So you might have to trust the code signing bits, the email and s/mime bits, etc, when you really only want to trust an intermediate certificate for web server authentication.

Several other tools fail here as well: fetch(1), svn(1).

Yeah, it sounds like other tools are not strictly following the RFC.

@michael-o is conflating unrelated details. The -partial_chain CLI flag and its API equivalent exist to allow applications to choose to tolerate peer provided certificate chains that don't include a full list of intermediate issuers. In particular this allows one to configure a peer-specific trust store with just the expected EE certificate (even if not self-signed) and match just that EE cert. Other choices are possible, for various scenarios.
Support for partial chains is off by default. And yet, even with the default full-chain policy, intermediate certificates are sometimes useful in the trust store, because they can be "better suited" to completing a path to a trust-anchor than the issuer certificates provided by some clients (e.g. be unexpired replacements). Of course this is a two-edged sword, and intermediate CA certs in an unmaintained trust store can be a liability.
Anyway, Michael's comments are largely off-topic in this thread, his trust store was missing a TA he needed for a particular server to verify.  Not related to support for partial chains, and at this point there's nothing new to say on that well-worn topic.
There is also apparently insufficient awareness of OpenSSL's "TRUSTED CERTIFICATE" feature, that allows the maintainer of a trust store to explicitly augment certificates (be they intermediate issuers or self-signed) as trusted for some set of (or all) purposes, making them explicit trust anchors (without requiring -partial_chain from the application).
See the documentation of x509(1) options: -addtrust, -clrtrust and -trustout.  If someone wants to fully trust an intermediate CA and not its issuers, it can be added to the trust store as an explicit trusted CA (e.g., with serverAuth as the trusted purpose).  And the same can be used to constrain the scope of otherwise trusted for everything WebPKI CAs, by explicitly labeling them as trusted for specific purposes (EKUs).
Over and out...

I'm closing this issue. The discussion is going around in circles, and this is unlikely to change IMHO.

You should write more clearly what you see as a problem here.

My expectation that default behavior is to verify the entire chain and not stop when the first intermediate is validated.

According to the output, the verification did not stop in between, but at the final cert in the chain where it cannot find the root cert in the trust store: the "Siemens Root CA V3.0 2016" which you did not place there for whatever reasons.

Your trust store should/must not contain intermediate CA certificates, but just trusted (root) CA certs.

Why? This isn't clear.

This is meanwhile answered above.

Hope you understand the difference between root CA certs and intermediate CA certs? The latter are not trusted but are needed for chain building. Moreover, they are usually provided by the peer - so also from this perspective it makes little sense to put them in the trust store.

Yes, I do understand why where should those be provided by the peer? None of the certs signed by our CA contain the intermeidate(s) as DER, the subject only of course.

Apparently you do not really understand how a certificate hierarchy works and is used.

authors: DDvO beldmit kaie kroeckx michael-o misery noloader richsalz t8m vdukhovni


text:
mac: add NULL checks to poly1305 and siphash.

Fixes #10612

Travis isn' relevant.

Merged, thanks.

source: gharchive/pull-request


text:
Implementation of Russian GOST CMS

Checklist

[x] documentation is added or updated
[ ] tests are added or updated

Ping @openssl/otc

All the comments seem to be processed now.

Travis is happy.

Ping @slontis @openssl/otc

@levitte, @mattcaswell - could you please take a look at it?

Travis failure is a timeout :(

@slontis, what about a final effort?

LGTM - A second review from someone else would be good though

Sure. But I've almost lost hope.
Many thanks!

Merged. Thanks!


text:
Deprecated EVP_PKEY_CTX_get0_dh_kdf_ukm() and EVP_PKEY_CTX_get0_ecdh_kdf_ukm()

The functions are not needed and require returning octet ptr parameters
from providers that would like to support them which complicates provider
implementations.
Fixes #12985
Checklist

[x] documentation is added or updated
[ ] tests are added or updated

Fixup pushed for the nits. I do not see a need to re-approve as it's trivial change.

Agreed.

Rebased to solve a simple merge conflict. @paulidale still OK?

Yes, still good.

@paulidale I pushed one more minor fixup

Just saw it, all good still.

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set.  Please review the updates and set the label manually.

Merged to master. Thank you for the reviews.

authors: openssl-machine paulidale t8m


text:
Revert "Useless allocation"

The memory allocation of siglen in a separate buffer is actually
required to run speed in async mode.
Removing the allocation causes failures with DSA and ECDSA
This reverts commit 0930e07d1eb522e663abe543ee865a508749946e.
More info
We observed a failure when running speed with async jobs in the latest beta release.
> openssl speed -elapsed -async_jobs 1 dsa512
You have chosen to measure elapsed time instead of user CPU time.
Doing 512 bit sign dsa's for 10s: 70486 512 bit DSA signs in 10.00s
DSA verify failure.  No DSA verify will be done.
OpenSSL 1.1.0-pre6 (beta) 4 Aug 2016
built on: reproducible build, date unspecified
options:bn(64,64) rc4(16x,int) des(int) aes(partial) idea(int) blowfish(ptr)
compiler: gcc -DDSO_DLFCN -DHAVE_DLFCN_H -DOPENSSL_THREADS -DOPENSSL_NO_STATIC_ENGINE -DOPENSSL_PIC -DOPENSSL_IA32_SSE2 -DOPENSSL_BN_ASM_MONT -DOPENSSL_BN_ASM_MONT5 -DOPENSSL_BN_ASM_GF2m -DSHA1_ASM -DSHA256_ASM -DSHA512_ASM -DMD5_ASM -DAES_ASM -DVPAES_ASM -DBSAES_ASM -DGHASH_ASM -DECP_NISTZ256_ASM -DPOLY1305_ASM -DOPENSSLDIR="\"/usr/local/ssl\"" -DENGINESDIR="\"/usr/local/lib64/engines-1.1\""  -Wa,--noexecstack

I believe that this is caused by the following commit: https://github.com/openssl/openssl/commit/0930e07d1eb522e663abe543ee865a508749946e
The changes were not necessary and the allocation of siglen in a separate buffer is actually required for the async jobs to work correctly.
After this commit the siglen is stored directly in tempargs.
When we start the ASYNC_JOB the struct is copied into the new execution context and all the changes done to siglen are visible only inside the ASYNC_JOB.
When the ASYNC_JOB finishes the async infrastructure does not copy the struct back in speed.
This is a design decision and not a bug in the async code.
In order to solve the error we need to pass siglen as a pointer so that the correct value is visible also from the caller (i.e. speed) and the verify operation can complete successfully.
Reverting the commit solves the problem:
git revert 0930e07d1eb522e663abe543ee865a508749946e
> openssl speed -elapsed -async_jobs 1 dsa512
You have chosen to measure elapsed time instead of user CPU time.
Doing 512 bit sign dsa's for 10s: 70485 512 bit DSA signs in 9.99s
Doing 512 bit verify dsa's for 10s: 119001 512 bit DSA verify in 10.00s
OpenSSL 1.1.0-pre6 (beta) 4 Aug 2016
built on: reproducible build, date unspecified
options:bn(64,64) rc4(16x,int) des(int) aes(partial) idea(int) blowfish(ptr)
compiler: gcc -DDSO_DLFCN -DHAVE_DLFCN_H -DOPENSSL_THREADS -DOPENSSL_NO_STATIC_ENGINE -DOPENSSL_PIC -DOPENSSL_IA32_SSE2 -DOPENSSL_BN_ASM_MONT -DOPENSSL_BN_ASM_MONT5 -DOPENSSL_BN_ASM_GF2m -DSHA1_ASM -DSHA256_ASM -DSHA512_ASM -DMD5_ASM -DAES_ASM -DVPAES_ASM -DBSAES_ASM -DGHASH_ASM -DECP_NISTZ256_ASM -DPOLY1305_ASM -DOPENSSLDIR="\"/usr/local/ssl\"" -DENGINESDIR="\"/usr/local/lib64/engines-1.1\""  -Wa,--noexecstack
                  sign    verify    sign/s verify/s
dsa  512 bits 0.000142s 0.000084s   7055.6  11900.1

ping @FdaSilvaYY .  looks like we goofed.

So running the test with the -async flag is enough to find the error?

Yes, running speed with async_jobs enabled and the SW engine should be enough to reproduce the error.
Note that the memory for the variable siglen is not actually initialized and if you are particularly (un)lucky sometimes the verification is successful.

Can you add another commit that sets *(loopargs[i].siglen) = 0 after the app_malloc line?

Done! @richsalz

Hi,
I'm looking at it .
The first issue is that loopargs are memcpy by ASYNC_stat_job, yes.
And the code was doing an implicit sharing as most of the loopargs structure content is pointers.

DSA_sign output/signature len siglen is not constant

Correct!
This is why we need to make sure that the value calculated inside the JOB is also used in DSA_verify

Closing this in preference to #1464.

authors: FdaSilvaYY agrandi mattcaswell richsalz


text:
crypto/store/ossl_result.c: Better filtering of errors

The diverse variants of try_XXX() were filtering errors independently
of each other.  It's better done in ossl_store_handle_load_result()
itself, where we have control over the overall success and failure of
the attempts.
Fixes #14973

As #14973 is labeled as urgent, shouldn't we merge this PR?

Yeah, I'll merge it now

Merged
e36a4dc476448a2ef212d774be48ce38ea6eb6df EVP: evp_keymgmt_util_try_import() should clean up on failed import
521a0bf6a11c4cdaef331934e93581d06ce834e1 crypto/store/ossl_result.c: Better filtering of errors
3d80b5e611f112fd004a4320cb5ecce93c73b7d4 STORE: Simplify error filtering in der2obj_decode()
a70936a8453a307992820f2a9d3e252f6c4f9ad6 TEST: correct test/recipes/30-test_evp_data/evppkey_ecdh.txt

authors: beldmit levitte


text:
sm2: fix error raise to not fail make update

An old style error function code is used and the name is wrong.  This breaks make update and is hence urgent.

Merged, thanks.


text:
APPS load_cert_certs(): fix crash on pcerts == NULL

One of the two defects was found by Coverity:
>>>     CID 1497107:  Null pointer dereferences  (FORWARD_NULL)
>>>     Dereferencing null pointer "pcerts".
696             OSSL_STACK_OF_X509_free(*pcerts);

Closing this in favor of the earlier #17488.


text:
Fix incorrect error branch in ossl_bn_rsa_fips186_4_derive_prime()

BN_priv_rand_range_ex() and BN_add() both return a 0 on failure and a 1 on success. In case of failure, the algorithm should fail. However, the branch that it goes through on failure is "goto end", not "goto err". Therefore, the algorithm will return 1 which indicates success instead of 0 for failure, leading to potential problems for the callers. Fix it by changing the goto to "goto err" instead of "goto end".
CLA: trivial
Please note that I found this using a static analysis tool I am developing at the moment. It could therefore be a false positive bug. I manually reviewed the case to be extra sure that it is a real bug.

Checklist

[ ] documentation is added or updated
[ ] tests are added or updated

This pull request is ready to merge

Merged to master, openssl-3.1 and openssl-3.0. Thanks for your contribution!

authors: nielsdos openssl-machine tmshort


text:
Nameopt fixes

This pull request unifies -nameopt flag processing in command-line apps and reduces usage of the X509_NAME_online function.

Sorry, to early to create pull request.


text:
Remove some logically dead code

This dead code should have been removed as part of #5874 but got missed.
Found by Coverity.

Checklist

[ ] documentation is added or updated
[ ] tests are added or updated

Yeah it was a coverity issue.


text:
edpm values template for beta

The hci template can be used here too, adding symlink for beta scenario.
As a pull request owner and reviewers, I checked that:

[x] Appropriate testing is done and actually running

/approve

/retest

authors: cjeanner dpinhas


text:
[VA] Deploy a single SSH key for the dataplane

It seems that the dataplane-operator/baremetal operator is/are having issues building the authorized_keys content of the cloud-init configuration, making BM instances cloud-init to fail when booting.
As a pull request owner and reviewers, I checked that:

[x] Appropriate testing is done and actually running
[x] Appropriate documentation exists and/or is up-to-date:

[x] README in the role
[x] Content of the docs/source is reflecting the changes

/approve
Could successfully test it against va-hci env.
This patch here is mostly a workaround before we get https://github.com/openstack-k8s-operators/openstack-baremetal-operator/pull/164 in.

/lgtm

Did you raise a bug?
Authorized keys in cloud-config should be a list.
https://cloudinit.readthedocs.io/en/latest/reference/modules.html#ssh
AFICT from the code it is setting it as a string.
templateParameters["AuthorizedKeys"] = strings.TrimSuffix(string(sshSecret.Data["authorized_keys"]), "\n")

    ssh-authorized-keys: {{ .AuthorizedKeys }}

https://github.com/openstack-k8s-operators/openstack-baremetal-operator/blob/9a04c56acdf52d1c309b656dfdd4025a95320926/pkg/openstackbaremetalset/baremetalhost.go#L73C3-L73C109
https://github.com/openstack-k8s-operators/openstack-baremetal-operator/blob/9a04c56acdf52d1c309b656dfdd4025a95320926/templates/openstackbaremetalset/cloudinit/userdata#L7

authors: cjeanner hjensas pablintino viroel


text:
Ignore non-vars file in fetch_compute_facts

Some files may be backups of previous versions, like the networking-environment-definition if the mapper is called twice. To avoid trying to load old backed up content load only the default files (yaml and json).

recheck

/lgtm

authors: frenzyfriday pablintino


text:
Move glance to the new API definition

GlanceAPI has been simplified by patches [1][2], and the implication of those patches is a change in the CRD used to define the glanceAPI service.
This patch aligns both the existing documentation and the test suite to work with the new way of deploying glance.
** Marking as a draft as [1] and [2] are still under review **
[1] https://github.com/openstack-k8s-operators/glance-operator/pull/329
[2] https://github.com/openstack-k8s-operators/openstack-operator/pull/514

can we undraft it?

Done, let's see the CI in action now :D

It seems consistent with the change we made and the new API have been adopted [1]
[1] https://logserver.rdoproject.org/87/187/1bc22ffde5259b408364beb912dde9a5f5a3539e/github-check/data-plane-adoption-github-rdo-centos-9-extracted-crc/2b4dae3/controller/crs/glances.yaml


text:
Drop make docs from pre-commit hooks

Different versions of ruby/bundler may yield different outputs, removing it for now

/test openstack-operator-build-deploy-kuttl

/test openstack-operator-build-deploy-kuttl


text:
Empty commit to make place for the v0.1.0 tag

To be able to distinguish between pseudoversions from main and from
18.0.0-proposed we need to start tagging main with v0.1.0 while keeping
18.0.0-proposed using the v0.0.x tags. Naturally we would only need to
start tagging after the first commit arrives to main. But to avoid
forgetting the tagging at that point we add an empty commit and tag it
instead.

lgtm, let me merge this later today, there is one or 2 commits we need to land in master and rebase the 18.0.0-proposed branch

authors: gibizer kopecmartin


text:
Add 'disk' as an alias of 'hd' ('disk' is used by ipmitool)

I figured it was worth adding this as an alias since 'ipmitool chassis bootdev disk' is familiar syntax from the systems side of things.

Thank you for contributing to openstack/pyghmi!
openstack/pyghmi uses Gerrit for code review.
If you have never contributed to OpenStack before make sure you have read the
getting started documentation:
http://docs.openstack.org/infra/manual/developers.html#getting-started
Otherwise please visit
http://docs.openstack.org/infra/manual/developers.html#development-workflow
and follow the instructions there to upload your change to Gerrit.

authors: klausfiend openstack-gerrit

repo: openstack/pyghmi

license_source: bigquery


text:
Bird hide mapping in ID

Hello,
Sorry if this is not the appropriate place for this kind of suggestion, but I'm not a regular on this platform (first message in this community).
As an ornithologist, I'm interested in mapping public bird hide (http://wiki.openstreetmap.org/wiki/Tag:leisure%3Dbird_hide).
If possible, I would like that ID (which is for me a great editor, congratulations !!!) offers appropriate tags to this object category (and possibly also in my language, which as you can see, is not English!).
However, I do not know how to participate in enriching ID on this feature.
Thank you to you for your advice.
As this is a forum for suggestions, here's my suggestion:
Add the bird hide mapping in ID (tag leisure = bird_hide)
If you want, I can give you a specific rendering (png icon).
Sylvain M.

Thanks @sylvain-m this is the right place.. I'm glad you like the editor! :)
We can add a preset for leisure=bird_hide.  I don't see a suitable icon in maki, so it will probably just look like a generic marker for now..

We can add a preset for leisure=bird_hide
Great ! Thank you very much.
In french, it is "Observatoire ornithologique". Could you add the french preset to ?
I don't see a suitable icon in maki
As I said, i could propose a marker that would be suitable (based on binocular, because this places are made for binocular owners). Where can i do it for maki ?
Thanks !

Sylvain M.

In french, it is "Observatoire ornithologique". Could you add the french preset too ?

All of iD's presets are translatable on Transifex to many different languages.  I'll add the preset now, and it should appear there for translation sometime tonight.

As I said, i could propose a marker that would be suitable (based on binocular, because this places are made for binocular's owners). Where can i do it for maki ?

You can propose a new Maki icon here.

Thank you very much for this improvement ! :)
I've made the translation in Transifex.
(i didn't know the term "machan" : could you tell me what it mean ?)
In how much time do you think this improvement will be operational in ID ?

(i didn't know the term "machan" : could you tell me what it mean ?)

It just means Bird Hide:  https://en.wikipedia.org/wiki/Bird_hide

In how much time do you think this improvement will be operational in ID ?

Probably this month.  We don't have anything planned yet, but it is good to let translators catch up before we make a new release.

authors: bhousel sylvain-m

repo: openstreetmap/iD

license: ISC