New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Property implementation #8224
Property implementation #8224
Conversation
I added this PR to the newly created github project "3.0 New Core + FIPS" Also, you need to rebase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, would you mind adding internal documentation for the functions declared in include/internal/property.h
, i.e. a manpage in doc/internal/man3
?
I removed the FIPS label, as this isn't the FIPS module per se. I believe adding this to the "3.0 New Core + FIPS" is a better way to mark these sorts of PRs |
typedef struct { | ||
const char *prop; | ||
PROPERTY_LIST *defn; | ||
char body[1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"unwarranted chumminess with the compiler" (web-search it)
Pay the price of portable code and make this a pointer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double allocating and ruining cache coherency in code that's meant to be fast to avoid something that's always worked...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's a misguided/premature optimization, at the expense of more clear code. We're going to be dispatching to crypto code :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C99 allows using char body[]; Is C99 viable target for OpenSSL 3.0.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't use C99. However, this code is portable and safe. There are two restrictions: don't use an array of such structures (no space to use between records) and don't store a type other than char there (alignment).
( ',' ( '-'? PropertyName | PropertyName ( '=' | '!=' ) Value ) )* | ||
Value ::= NumberLiteral | ||
| StringLiteral | ||
StringLiteral ::= QuotedString | UnquotedString |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think unquotedstring is a very big mistake. It makes it really hard to add keywords to the query language in the future. Look at perl's "barewords" evolution, for example.
Value ::= NumberLiteral | ||
| StringLiteral | ||
StringLiteral ::= QuotedString | UnquotedString | ||
QuotedString ::= '"' [^"]* '"' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should allow backslash-escape inside the quoted string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? so we can write fips="Y\e\s"
?
I'd be happy to ban \ inside a string to provide a capability to add it painlessly in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Banning it for future expansion helps. But I meant to only escape quotes, as in things like "Hello "world""
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If anyone is ever going to want to have a Windows path in a quoted string, we'd better not use \ as the escape character.
That being said, the \ is more or less universally known as an escape character, and is already used as such elsewhere in OpenSSL, so it's actually a little surprising to have something different for property values.
(and I'm curious, where was ^ inspired from? That's actually used in VMS file specs, where you'll see things like openssl-1^.0^.2q^.tar.gz
, but I've not seen you as VMS savvy, so...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where was ^ inspired from?
The caret at the beginning of a character class inverts the set, i.e. [^a-b]
is the complement of [a-b]
. If you want to include a literal caret, it must not be first in the set. See GREP(1).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Of course you know that, it's part of your Perl-fu. It was just too early in the morning... ;-) ).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
D'oh.
And still, that was after the first coffee.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the ^ is stretching eBNF a little.
crypto/property/README
Outdated
names and values to small integer indices. Names and values are | ||
stored in separate hash tables. The two Boolean values "yes" and "no" | ||
are populated as the first two members of the value table. The string | ||
"name" is automatically populated as the first property name, to enable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does this happen? I couldn't see it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See ossl_property_parse_init
in property_parse.c
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That function only seems to define "provider", "version", "fips" and "engine", but not "name"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The question is which names are we going to reserve?
I changed "name" to "provider" since it seemed more applicable but missed the README.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix is to not say which property names are pre-loaded, just to say that they are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can have the discussion about which get added later. I expect some worthwhile ones will naturally appear over the course of the project.
* space and time efficient algorithms if this becomes a bottleneck. | ||
*/ | ||
|
||
typedef struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnamed struct isn't common. also same bad chumminess thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnamed struct might be uncommon, but naming it becomes a bit silly if that name is never used. I see no reason to add a struct name now.
Looking through the code again and its intended use, I'm unsure that the functions declared in That store (or those stores) are going to be used to save away methods, i.e. instances of EVP_CIPHER, EVP_MD, etc. Do we call them implementations or do we call them methods? Historically, they were called the latter (it's even more visible if you consider EVP_PKEY_METHOD), so I wonder of Food for thought. |
crypto/property/property.c
Outdated
|
||
ossl_impl_store_cleanup(); | ||
return 0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure that this is called automatically, either as part of the init process, or in here with the help of a RUN_ONCE function (see #8225 to see how I do that)
crypto/property/property.c
Outdated
ossl_prop_defn_cleanup(); | ||
ossl_impl_store_free(g_implementations); | ||
g_implementations = NULL; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure that this is called automatically, either as part of the cleanup process, or in here with the help of OPENSSL_atexit()
(see #8225 to see how I do that)
82ed864
to
88c0401
Compare
I've addressed most of the comments (resolving those that I did fix). |
Settled. |
Fixed in a different PR. |
Tests are updated so that they don't reinitialise the properties anymore. |
The CIs pass :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't think we should have bare PROPERTY and it should have an OSSL or OPENSSL prefix.
Which PROPERTY ? Everything exposed outside of the crypto/property directory starts OSSL_ doesn't it? All functions exposed from crpyto/property also start ossl_. Some of the types don't, are they realistically a problem? |
You have ossl_ prefixes on the functions expected to be used outside of the property directory but no prefix on the typedefs. They should be OSSL_ ... basically we don't want to run into clashes with other code and PS_IDX and PROPERTY are likely to clash. Yes I know these aren't currently expected to be visible elsewhere - but they may evolve that way - and we should be clean in our "namespace" usage IMHO. |
I've prefixed the internal symbols. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once the CIs show clear for the most recent change - approved.
@levitte @mattcaswell I'll merge this in the morning (my time) unless there are objections. |
Squashed... |
I'd like to run a test tomorrow against things I have that build on this... |
(It looked good visually, though...) |
Let me know when you're done and I'll merge it or merge it yourself if the time-zones are immiscible. |
Properties are a sequence of comma separated name=value pairs. A name without a corresponding value is assumed to be a Boolean and have the true value 'yes'. Values are either strings or numbers. Strings can be quoted either _"_ or _'_ or unquoted (with restrictions). There are no escape characters inside strings. Number are either decimal digits or '0x' followed by hexidecimal digits. Numbers are represented internally as signed sixty four bit values. Queries on properties are a sequence comma separated conditional tests. These take the form of name=value (equality test), name!=value (inequality test) or name (Boolean test for truth). Queries can be parsed, compared against a definition or merged pairwise.
Properties are a sequence of comma separated name=value pairs. A name without a corresponding value is assumed to be a Boolean and have the true value 'yes'. Values are either strings or numbers. Strings can be quoted either _"_ or _'_ or unquoted (with restrictions). There are no escape characters inside strings. Number are either decimal digits or '0x' followed by hexidecimal digits. Numbers are represented internally as signed sixty four bit values. Queries on properties are a sequence comma separated conditional tests. These take the form of name=value (equality test), name!=value (inequality test) or name (Boolean test for truth). Queries can be parsed, compared against a definition or merged pairwise. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tim Hudson <tjh@openssl.org> (Merged from #8224)
Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tim Hudson <tjh@openssl.org> (Merged from #8224)
Merged, thanks for the extensive and positive feedback. |
... someone's in a rush. 😉 My tests ran well after some appropriate mods. The manual bits need some fixups, something I discovered moments ago... |
In a rush or needing a rush? I waited until I thought I’d be asleep (which I have been). That I’m not indicative is indactive of a non-problem….
Pauli
--
Dr Paul Dale | Cryptographer | Network Security & Encryption
Phone +61 7 3031 7217
Oracle Australia
… On 18 Feb 2019, at 6:00 pm, Richard Levitte ***@***.***> wrote:
... someone's in a rush. 😉
My tests ran well after some appropriate mods. The manual bits need some fixups, something I discovered moments ago...
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub <#8224 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ARFiwdynDshahU7-Wch_GVzyCWHeZRDOks5vOl2IgaJpZM4a5LHK>.
|
Care to look at #8265? |
A first pass at implementing some efficient property definition and query infrastructure.
Properties are defined by providers as strings which are parsed into name, value pairs.
Queries are parsed into name relation value triplets.
Implementations that match the query are considered for further use whereas implementations that don't match are immediately discarded from consideration.
The property system, allows global properties to be specified in addition to local properties (which can override globals).
If there is a match, there are capabilities for caching the outcome which will improve performance in the future.
The implementation database permits several interfaces to operate within:
The generate usage pattern is: