Ada wrapper incorrectly sets verification method for remote peers

[dalybrown]

I think there is a bug in the logic for setting the verification method in the Ada wrapper here.

The documentation suggests it should be a logical OR operator here.

However, it performs an AND operator here.

Unless the documentation is incorrect! But the C examples suggest it is a logical OR here.

[dgarske]

@joakim-strandberg can you please review this bug report?

[joakim-strandberg]

@dalybrown good catch! The Ada code should be a direct translation of the C examples and that translation does not look correct. My bad, thanks for finding it!!

The best way to fix it? One idea is to rename the function 'function "&" (Left, Right : Mode_Type) return Mode_Type;' to 'function "|" (Left, Right : Mode_Type) return Mode_Type;' and instead of "and" in the implementation "or" should be used instead. It will make the Ada code look more like the C implementation when using the wolfSSL library. What do you think?

[dalybrown]

@dalybrown good catch! The Ada code should be a direct translation of the C examples and that translation does not look correct. My bad, thanks for finding it!!

The best way to fix it? One idea is to rename the function 'function "&" (Left, Right : Mode_Type) return Mode_Type;' to 'function "|" (Left, Right : Mode_Type) return Mode_Type;' and instead of "and" in the implementation "or" should be used instead. It will make the Ada code look more like the C implementation when using the wolfSSL library. What do you think?

My preference would be to use the existing Ada syntax for logical operators: and, or, and xor. So in this case, my preference would be to define the or operator for the Mode_Type.

In general, I like things to not look like C haha... but that's my opinion.

[dalybrown]

p.s., there is another issue too with examples: the user_settings.h needs to be updated to include #define HAVE_NETDB_H. This was introduced in a subsequent commit.

[dalybrown]

p.p.s, I'm happy to fix both issues here. Let me know.

[dgarske]

Hi @dalybrown , yes feel free to put up a PR fixing both issues. You are right about needing HAVE_NETDB_H. That normally comes from configure in the config.h.

[joakim-strandberg]

My preference would be to use the existing Ada syntax for logical operators: and, or, and xor. So in this case, my preference would be to define the or operator for the Mode_Type.

It's reasonable to use the "or" operator instead of the "|" operator. You can choose either way @dalybrown. I do note that we are in a gray zone. The Ada language uses both the keyword "or" and "|" to list alternatives. In if-statements the "or" keyword is used, for example "if A or B then" but in case-statement over an enumeration type the "|" is used for example "when Red | Green | Blue =>". In the C version of the wolfSSL library it is known that the Method_Type is an unsigned 32-bit number and one uses bit-wise operations to turn on specific flags. In the Ada version of the wolfSSL library the Method_Type is a private type indicating to the user that the exact type of a Method_Type instance is unknown. It's a fundamental idea in Ada to hide exact representations from users as much as possible to limit potential errors. Classical example is the Integer type where one does not have access to the representation. The major argument for that is the code should be cross-platform and negative numbers may be represented in different ways depending on the hardware. So, in an Ada binding the Method_Type should be private, so it is not known what Method_Type is, but then using the "or" operator to list alternatives implicates that the Method_Type is actually a modular type defined in the Interfaces package and the "or" operator defined there is used under the hood. Which is true, but then again I am thinking is it possible to hide this fact and still have an Ada binding that is easy to use and understand? Just something to keep in mind. I don't think we should spend time on this. It's good to be pragmatic, pushing things forward, everything doesn't have to be perfect.

[dalybrown]

That's a good point. WolfSSL happens to use a logical operator here. There is probably a more idiomatic Ada way of specifying this than having clients do any logical operations.

I'll think about it for a bit and circle back with a few options.

One way would be to track the state of the mode that is set, and have a subprogram that you continually set the flags. Under the hood it will perform the logical or operation but that's not exposed to the client.

type State is private;
procedure Set (This : Mode_Type; In_This : in out State);

private

type State is Unsigned_32;

and then in the body you could just the operator:

procedure Set (This : Mode_Type; In_This : in out State)
is
begin
   In_This := In_This or Unsigned_32 (This);
end Set;

You can then just call it for every flag you want to set and then call the regular procedure.

There are probably many other options. Thoughts?

[dalybrown]

Maybe better names would be:

procedure Include (This : Mode_Type; With_These : in out Flags);

Or something...

[dgarske]

Hi @dalybrown , will you be able to put up a PR to fix this mask and the netdb_h?

[dalybrown]

Hi @dalybrown , will you be able to put up a PR to fix this mask and the netdb_h?

Definitely! I was hoping to hear back from @joakim-strandberg on my suggestions prior to pushing any changes.

[dgarske]

Hi @dalybrown ,

Is this issue resolved? Let me know if you need anything.

Thanks ,
David Garske, wolfSSL

[dalybrown]

Ah I didn't get back to this yet - sorry! It's been a busy summer. I haven't forgotten - will get back to this hopefully in September.

[dgarske]

Hi @dalybrown or @joakim-strandberg

It looks like that bug with the WolfSSL_CTX_Set_Verify "AND" is still there. Are either of you able to work on resolving this? If not please let me know and we can assign internally.

Thanks,
David Garske, wolfSSL

[dalybrown]

Hi,

Sorry for the inaction.

I plan to eventually get to this, but truthfully I don't know when. It's an easy enough fix, but unfortunately I've been too busy to circle back.

How about this: if I don't fix by the new year, someone internal can take a look.

Let me know!

Cheers,

Daly

[LinuxJedi]

Hi @dalybrown,

I have opened a pull request which should fix the issue. Thank you for reporting it.

Andrew (LinuxJedi) Hutchings, wolfSSL