Certificate: Remove subject name uniqueness validation
RFC 6487:
> An issuer SHOULD use a different subject name if the subject's key
> pair has changed (i.e., when the CA issues a certificate as part of
> re-keying the subject.)
Fort's implementation was problematic. The code was comparing the
certificate's subject name and public key to siblings that had
potentially not been validated yet. It seems to me this would make it
possible for attackers to crash FORT (by posting invalid objects) or to
invalidate legitimate objects (by publishing siblings that contained
conflicting subject names and public keys, without having to worry about
the rest of the fields).
This would be somewhat difficult o fix. I asked on the mailing list and
Discord ("RPKI Community"), and it seems the concensus is "don't
validate it." Subject Names don't really matter that much, because
RPKI's primary concern is resource ownership, not identity. Furthermore,
I'm not convinced that chopping off branches off the tree because of a
clumsy key rollover is a good idea.
libcrypto: Make ASN1_IA5STRING access more consistent
Honestly, this is probably a waste of time. Even this way of accessing
the API has noticeable risks. There is no such thing as following good
programming practices when libcrypto is involved.
Some URIs don't have meaningful local or global paths. I checked the
call hierarchy, and the code sensibly steers away from the corresponding
getters in those cases.
I added null checks, to future-proof the constraint.
Fort was reinitializing the CURL object on every HTTP download.
libcurl's documentation recommends against this:
You need one handle for each easy session you want to perform.
Basically, you should use one handle for every thread you plan to
use for transferring.
(...)
If you then want to transfer another file, the handle is ready to be
used again. Mind you, it is even preferred that you re-use an
existing handle if you intend to make another transfer. libcurl will
then attempt to re-use the previous connection.
The CURL was moved to the thread state variable, is initialized once per
thread, per validation cycle, and is employed in every HTTP transfer.
Goddamnit. This started as a couple of RRDP bugfixes that sprawled into
another massive refactor thorough.
Lots of stuff was simplified. The RRDP code was pretty filthy; some
functions tried to do too much at once, error codes were being
transformed unnecessarily, argument lists were too long, there were
outdated comments, and some awkward logic suggests clumsy and misguided
bugfixes have taken place.
I know I fixed the following bugs, but the refactor was very aggressive.
I have no hope of this list being exhaustive:
**** Corrupted HTTP download on retry ****
If an HTTP dowload failed mid-transfer, Fort would end up appending the
file to itself on retry, because the file descriptor wasn't reset.
**** HTTP code 304 being treated as an error ****
So the "If-Modified-Since" HTTP header was pure liability.
**** Per-TAL namespaces, not per-RPP namespaces ****
This is the original problem:
https://mailarchive.ietf.org/arch/msg/sidrops/FrAjMFWY5a_cofpOoCEO5Yr_ZLI/
Basically, RRDP files (snapshots and deltas) declare URIs for their
contained files (RPKI objects), and there's nothing intrinsically
stopping a malicious CA's RRDP file from overriding some other CA's RPKI
object. So the RP needs to create per-RPP namespaces.
Existing solution was using per-TAL namespaces. This prevented CAs from
one tree from overriding files from a different tree, but not files on
the same tree.
**** Bad fnstack management ****
Some RRDP error messages were referencing the wrong file, because the
file name was pushed too late to the stack.
**** Was always redownloading all RRDP on startup ****
RRDP session IDs and serials were cached on RAM. On top of that,
RRDP files (including notifications) were always being deleted
immediately after use.
This meant that, on startup, there was absolutely no way of telling
whether an RRDP RPP was outdated or not, so the code had to download
everything all over again.
This was particularly egregious in standalone mode, because that counts
as *always* startup. Ugh.
I purged the session cache, as well as that weird eager delete gimmic.
Fort now queries existing notification files to find out RRDP sessions
IDs and serials.
The `root` and `root-except-ta` strategies were naive optimizations
because they assumed rsync was the only available transfer protocol.
They just cause redundant transfers now that RRDP not only exists,
but has actually eclipsed rsync.
rsync always works in `strict` mode now, and `rsync.strategy` is
deprecated and ignored.
**** `shuffle-uris` ****
I'm willing to return this one if someone argues in its favor, but I
strongly suspect nobody cares.
Weird, annoying, seemingly pointless feature. And I hope no one's using
it, because it encourages rsync when HTTP is obviously preferred.
RFC 8630:
In the case where a TAL contains multiple TA URIs, an RP MAY use a
locally defined preference rule to select the URI to retrieve the
self-signed RPKI CA certificate that is to be used as a TA. Some
examples are:
o Using the order provided in the TAL
o Selecting the TA URI randomly from the available list
o Creating a prioritized list of URIs based on RP-specific
parameters, such as connection establishment delay
It sounds to me the spec is just throwing out ideas, not mandating the
existence of this flag. We don't implement the third point anyway.
`shuffle-uris` is now deprecated and a no-op. TAL URIs are always
queried top-to-bottom, as this is assumed to be the RIR's preference.
It's gotten increasingly evident that I'm going to be maintaining this
project indefinitely and alone, so I feel like I need to move to a more
stateless model. I deleted or reimplemented the following tables:
- `reqs_errors`: This was very disproportionate overhead for a very
small (and kind of optional) feature (failed download logging on the
operation logs), which is going to be superceded by the Prometheus
endpoint anyway.
The Prometheus endpoint implementation has reached new levels of
urgent.
- `visited_uris`: I think the repository cache should be cleansed by way
of the "Accessed" cached files property, not from a list of "visited
URIs."
I'm still working out the details of this mechanism, but I (admittedly
recklessly) already deleted the existing code to simplify the
refactor.
- `db_rrdp_uris`: The validation cycle's temporary URI download status
cache aspect of this sounds useful, so I extracted it into a new
module: `rpp_dl_status`.
On the other hand, the RRDP session ID and serial cache aspect was,
as previously mentioned, problematic, so I removed it.
- `db_rrdp`: Its whole point was to cache the `db_rrdp_uris` and the
"RRDP workspace" over different validation cycles. Since neither of
them exist anymore (the latter was purged as part of the "per-RPP
namespaces" bugfix), `db_rrdp` naturally followed.
This is a big commit because most of the problems were intertwined. It's
also an unstable checkpoint; I left a bunch of TODOs, and no testing has
taken place. The rest can ship in proper incremental commits.
Helps the code review. Some structs and functions (such as
struct delta_router_key and router_key_print()) were bleeding into
mostly unrelated modules, and there were a couple of data types (struct
v4_address and struct v6_address) that were only used once, and induced
needless copying.
- Increase default remote object file size limit, because RRDP snapshot
files can be very large. (Current: ~148 MB, double on key rollover)
- Treat HTTP redirects as errors.
- Before: Redirects were treated as successes, but Fort didn't
bother to follow the redirect. As a result, the code was either
not finding the file, or finding an empty file.
- Now: Redirects are treated as errors. (Not sure if I'm meant to do
something here; curl doesn't do it automatically, and RFCs are
silent. In particular, I'm not in the mood to have to deal with
redirect loops and whatnot.)
- Remove ROA database clone operation in the SLURM code.
According to the code, it was working on a clone "so that updates can
be reverted" (on error, I presume.) But it was never reverting them.
- Refactor SLURM cleanup code.
I don't remember this one very well. Starting from the clone removal,
I got distracted with some inconsistent cleanups. Patched a buggy
cleanup somewhere.
There's still more I want to do to the SLURM code; it looks somewhat
slow.
It seems the #58 and #59 problem is a stray defer separator pop.
The comment above x509stack_cancel() clearly states that the function
should only be called shortly after a x509stack_push(), but there's one
in certificate_traverse() that isn't.
Removing this x509stack_cancel() seems to prevent the crash. I'm still
investigating the original intent of this code.
HTTP: Patch CURLOPT_LOW_SPEED_LIMIT and CURLOPT_LOW_SPEED_TIME
Likely due to some misunderstanding, Fort was managing both of these
variables using one command-line argument (--http.idle-timeout). This
unnecessarily limited the configurability of minimum transfer speeds for
HTTP connections.
--http.idle-timeout is now deprecated, and has been replaced by
--http.low-speed-limit and --http.low-speed-time, which correlate
verbatim to the corresponding curl arguments (CURLOPT_LOW_SPEED_LIMIT
and CURLOPT_LOW_SPEED_TIME).
It was allocating the deltas array twice, for seemingly no reason.
Also, the array slots were pointers, and the two arrays pointed to
different instances of the same objects. For seemingly no reason.
Now there's only one array, and it stores the objects directly.
1. Update the TAL URLs. (The old ones were very obsolete.)
2. Add --init-as0-tals. (Used to download the ASN0 TALs.)
3. Deprecate and zero-op --init-locations. (Didn't make sense.
If the user needs a different URL, they can do wget instead.)
4. Deprecate setup_fort.sh. (Seems to be redundant. --init-tals
already takes care of downloading TALs.)
In truth, this data structure should technically be a linked list.
But I'm not sure if sacrificing cache locality for faster removal is
worth the tradeoff.
RTR Server: thread-pool.server.max now refers to RTR requests
Apparently, there was a huge misunderstanding when the thread pool was
implemented.
The intended model was
> When the RTR server receives a request, it borrows a thread from the
> thread pool, and tasks it with the request.
Which is logical and a typical thread pool use case. However, what was
actually implemented was
> When the RTR server opens a connection, it borrows a thread from the
> thread pool, and tasks it with the whole connection.
So `thread-pool.server.max` was a hard limit for simultaneous RTR
clients (routers), but now it's just a limit to simultaneous RTR
requests. (Surplus requests will queue.) This is much less taxing to the
CPU when there are hundreds of clients.
Thanks to Mark Tinka for basically spelling this out to me.
-----------------------
Actually, this commit is an almost entire rewrite of the RTR server
core. Here's a (possibly incomplete) list of other problems I had to fix
in the process:
== Problem 1 ==
sockaddr2str() was returning a pointer to invalid memory on success.
Was "keep track of the clients, expire deltas when all clients outgrow
them." I see two problems with that:
1. It'll lead to bad performance if a client misbehaves by not
maintaining the connection. (ie. the server will have to fall back to
too many cache resets.)
2. It might keep the deltas forever if a client bugs out without killing
the connection.
New conditional is "keep deltas for server.deltas.lifetime iterations."
"server.deltas.lifetime" is a new configuration argument.
== Problem 3 ==
Serials weren't being compared according to RFC 1982 serial arithmetic.
This was going to cause mayhem when the integer wrapped.
(Though Fort always starts at 1, and serials are 32-bit unsigned
integers, so this wasn't going to be a problem for a very long time.)
== Problem 4 ==
The thread pool had an awkward termination bug. When threads were
suspended, they were meant to be ended through a pthread signal, but
when they were running, they were supposed to be terminated through
pthread_cancel(). (Because, since each client was assigned a thread,
they would spend most of their time sleeping.) These termination methods
don't play well with each other.
Apparently, threads waiting on a signal cannot be canceled, because of
this strange quirk from man 3 pthread_cond_wait:
> a side effect of acting upon a cancellation request while in a
> condition wait is that the mutex is (in effect) re-acquired before
> calling the first cancellation cleanup handler.
(So the first thread dies with the mutex locked, and no other threads
can be canceled because no one can ever lock the mutex again.)
And of course, you can't stop a server thread through a signal, because
they aren't listening to it; they're sleeping in wait for a request.
I still don't really know how would I fix this, but luckily, the problem
no longer exists since working threads are mapped to single requests,
and therefore no longer sleep. (For long periods of time, anyway.)
So always using the signal works fine.
It was pretty messy; I had to rewrite a good chunk of it.
== Problem 1 ==
It was discarding meaningful validation results when miscellaneous
errors prevented the deltas array from being built.
Deltas are optional; as long as Fort has the snapshot of the latest
tree, it doesn't technically need deltas. They speed up synchronization,
but in the worst case scenario, the RTR server can keep pushing Cache
Resets.
Severity: Warning. Memory allocation failures are the only eventuality
that might prevent the deltas array from being built.
== Problem 2 ==
The database was always keeping one serial's worth of obsolete deltas.
Cleaned up, saves a potentially large amount of memory.
Severity: Fine. Not a memory leak.
== Problem 3 ==
The code computed deltas even whene there were no routers listening.
Routers are the only delta consumers, so there was no need to waste all
that time.
Severity: Fine; performance quirk.
== Problem 4 ==
I found an RTR client implementation (Cloudflare's rpki-rtr-client) that
hangs when the first serial is zero. Fort's first serial is now 1.
Severity: Warning. This is rpki-rtr-client's fault, but any client
implementations are prone to the same bug. The new solution is more
future-proof.
== Problem 5 ==
It seems it wasn't cleaning the deltas array when all routers were known
to have bogus serials. This was the code:
/* Its the first element or reached end, nothing to purge */
if (group == state.deltas.array ||
(group - state.deltas.array) == state.deltas.len)
return 0;
If you reached the end of the deltas array, and the minimum router
serial is larger than all the array serials, then all deltas are
useless; you're supposed to purge all of them.
Severity: Fine. It was pretty hard to trigger, and not a memory leak.
init: Remove SIGPIPE SIG_IGN override during RTR server startup
Because it's now redundant.
As discovered in #49, SIGPIPE is total bullshit in all circumstances, no
exceptions, so there's now a global SIGPIPE SIG_IGN override, rendering
this one useless.
I guess we now know why #49 never triggered on server mode.
I think I finally found #49. The signal is not SIGSYS; it's SIGPIPE.
That's why dorpauli was getting no core dumps.
Apparently, this is a typical newbie trap for libcurl users.
CURLOPT_NOSIGNAL is incapable of supressing all SIGPIPEs, because some
of them are OS-generated.
I can't believe how dumb SIGPIPE has turned out to be. I/O functions
should return EPIPE; not interrupt the whole program to a handler that
defaults to "die silently."
These should be safe. They just print the signal number, the stack
trace, then restore the original behavior.
The only problem is I can't do the same with SIGKILL nor SIGSTOP,
but I suppose if SIGKILL were the problem, the kernel would have left
something in the logs. And SIGSTOP would have left the process alive.
ad841d50 was a mistake. It was never agreed in #40 that Fort should
shotgun blast its own face on the first ENOMEM, and even if it was, the
idea is preposterous. Memory allocation failures are neither programming
errors nor an excuse to leave all the routers hanging.
While there's some truth to the notion that a Fort memory leak (which
has been exhausting memory over time) could be temporarily amended by
killing Fort (and letting the OS clean it up), the argument completely
misses the reality that memory allocation failures could happen
regardless of the existence of a memory leak.
A memory leak is a valid reason to throw away the results of a current
validation run (as long as the admin is warned), but an existing
validation result and the RTR server must remain afloat.
Hypothesis: Something (which I haven't spotted yet) was causing the
main thread to skip its wait before the pool threads finished their
tasks. Maybe something to do with the ready signal again?
So the main thread returned early, which means pool threads were
silently suppressed by the OS. That explains the early terminations and
nonexistent stack traces.
If I keep finding crippling errors like this, I will definitely have to
purge the thread pool. It's turned out to be a fucking bug colony at
this point. I'm sick of it.
Way I see it, the root of the problem was the thread pool's control
code, which was too complicated for its own good. A surprisingly large
part of why it was overcomplicated was because it reinvented thread
joining.
So I simplified the control code by removing the detach property. Now
that the main thread joins the proper way, the validation code will not
be interrupted anymore.
This might well be the solution for #49. However, it bothers me that I
still don't have a reasonable explanation as to why the main thread
seemed to be skipping wait.
1. (error) Fix the --work-offline flag.
It has been unused since commit 85478ff30ebc029abb0ded48de5b557f52a758e0.
2. (performance) Remove redundant fopen() and fclose() during
valid_file_or_dir().
If stat() is used instead of fstat(), there's no need to open and
close the file.
(Technically, it's no longer validating readabilty, but since the
validator downloads the files, read permission errors should be
extremely rare, and can be catched later.)
3. (fine) Remove return value from thread_pool_task_cb.
This wasn't a problem, but the return value was meaningless, and
no callers were using it.
- Main validation loop: Remove some confusing, seemingly needless
wrapper functions.
- Libcurl: Catch lots of status codes properly
- Libcurl: Send proper data types to curl_easy_setopt()
(Argument types were not matching documented requirements.)
- RTR server: Reduce argument lists.
1. Merges the log and debug modules. I think their separation was the
reason why they forgot to add stack traces to syslog when they added
syslog to the project.
Not risking that mistake again.
2. Removes as many obstacles as possible from stack trace printing on
critical errors.
3. Add mutexes to logging. This should prevent messages from mixing on
top of each other when there are threads.
1. (warning) The libcrypto error stack trace was always showing empty.
This was because of a bad counter.
2. (critical) Normal stack traces were only being printed in the
standard streams, never on syslog.
This is probably the reason why we don't have a proper error message
on #49. It's probably a segmentation fault.
Also a whole bunch of cleanup. The logging module had a bit of a
duplicate code problem.
I accidentally removed a lock operation in the previous commit,
so lots of undefined behavior was being triggered.
Also, restores (but improves) the thread ready signal. It's hard to
explain:
- Before: Workers send ready signal to parent,
but parent might not be listening yet;
Therefore parent timeouts on wait.
- Previous: Workers do not send ready signal to parent.
Therefore, parent might signal work when no workers are ready yet;
Therefore nobody works.
- Now: Workers send ready signal to parent,
parent listens lazily (ie. late), but only if workers aren't ready
yet.
Therefore, correct behavior.
pcarana [Wed, 27 Jan 2021 15:32:18 +0000 (09:32 -0600)]
Fix rsync and thread pool bugs.
+Mistakenly (of course, it was a bug) the returned value from rsync execution was being confused with the returned value from execvp call. The main problem was when rsync returned a code 12 (Error in rsync protocol data stream); in that case, the caller confused that error with ENOMEM (also with value 12), which led to terminate execution.
+The thread pool wait function wasn't considering pending taks at the queue; also the poll function was holding and releasing the mutex more than it was needed, and the thread attributes are now globally initialized (thanks @ydahhrk for the code review).
+Increment the number of threads at the internal pool to 10.
pcarana [Wed, 25 Nov 2020 23:28:40 +0000 (17:28 -0600)]
Add docs for thread-pool.* args, add flow to reject RTR clients
+An RTR client is rejected when there aren't available threads at the pool to attend it.
+Add new function at thread_pool.c to check if the pool has available threads to work.
+Use an internal buffer at sockaddr2str(), since the buffer received as parameter wasn't utilized by anybody.
+Update max values: thread-pool.server.max=500, thread-pool.validation.max=100.
pcarana [Tue, 24 Nov 2020 00:20:40 +0000 (18:20 -0600)]
Use thread pool for RTR server/clients, validation cycles at main thread
+Change the previous logic: RTR server lived at the main thread and the validation cycles were run in a distinct thread. Now the validation cycles are run at the main thread, and RTR server is spawned at a new thread.
+Create internal thread pool to handle RTR server task and delete RRDP dirs tasks.
+Create thread pool to handle incoming RTR clients. One thread is utilized per client.
+Create args: 'thread-pool.server.max' (spawned threads to attend RTR clients) and 'thread-pool.validation.max' (spawned threads to run validation cycles).
+Shutdown all living client sockets when the application ends its execution.
+Rename 'updates_daemon.*' to 'validation_run.*'.
pcarana [Fri, 6 Nov 2020 01:49:43 +0000 (19:49 -0600)]
Implement a thread pool, still pending to use at RTR clients
+The pool is basically a tasks queue, it's initialized using a fixed amount of threads (all of them spawned at pool creation) where each of them will be waiting for pending tasks to attend.
+TODO: the number of threads per pool must be configurable.
+TODO: right now only a pool is utilized at the TALs validation (and therefore the whole RPKI tree beneath them), at least another pool can be used to receive RTR clients.
pcarana [Fri, 30 Oct 2020 20:18:13 +0000 (14:18 -0600)]
Add '--daemon' argument to daemonize fort, fixes #25
+When the flag is enabled, any value set at '--log.output' and '--validation-log.output' is overwritten with 'syslog' (all enabled logs will be sent to syslog).
+Update the docs to include the new argument.
pcarana [Wed, 28 Oct 2020 00:22:11 +0000 (18:22 -0600)]
Add argument '--init-tals' to fetch RIR TALs
+Once utilized, FORT tries to download the TALs and exits. In order to download ARIN TAL, the user must explicitly accept its RPA by typing yes (ignoring case) at stdin.
+Remove the write callback from HTTP download callers, it was unnecessary since every caller did the same thing.
+Update the docs to include the new argument.
projects / thirdparty / FORT-validator.git / log
